Re: [PATCH v9 05/16] qapi: net: add stream and dgram netdevs
On Wed, Oct 05, 2022 at 12:08:27PM +0200, Laurent Vivier wrote:
> On 9/28/22 07:55, David Gibson wrote:
> > > +static int net_stream_server_init(NetClientState *peer,
> > > + const char *model,
> > > + const char *name,
> > > + SocketAddress *addr,
> > > + Error **errp)
> > > +{
> > > +NetClientState *nc;
> > > +NetStreamState *s;
> > > +int fd, ret;
> > > +
> > > +switch (addr->type) {
> > > +case SOCKET_ADDRESS_TYPE_INET: {
> > > +struct sockaddr_in saddr_in;
> > > +
> > > +if (convert_host_port(_in, addr->u.inet.host,
> > > addr->u.inet.port,
> > > + errp) < 0) {
> > > +return -1;
> > > +}
> > > +
> > > +fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> > > +if (fd < 0) {
> > > +error_setg_errno(errp, errno, "can't create stream socket");
> > > +return -1;
> > > +}
> > > +qemu_socket_set_nonblock(fd);
> > > +
> > > +socket_set_fast_reuse(fd);
> > > +
> > > +ret = bind(fd, (struct sockaddr *)_in, sizeof(saddr_in));
> > > +if (ret < 0) {
> > > +error_setg_errno(errp, errno, "can't bind ip=%s to socket",
> > > + inet_ntoa(saddr_in.sin_addr));
> > > +closesocket(fd);
> > > +return -1;
> > > +}
> > > +break;
> > > +}
> > > +case SOCKET_ADDRESS_TYPE_FD:
> > > +fd = monitor_fd_param(monitor_cur(), addr->u.fd.str, errp);
> > > +if (fd == -1) {
> > > +return -1;
> > > +}
> > > +ret = qemu_socket_try_set_nonblock(fd);
> > > +if (ret < 0) {
> > > +error_setg_errno(errp, -ret, "%s: Can't use file descriptor
> > > %d",
> > > + name, fd);
> > > +return -1;
> > > +}
> > > +break;
> > > +default:
> > > +error_setg(errp, "only support inet or fd type");
> > > +return -1;
> > > +}
> > > +
> > > +ret = listen(fd, 0);
> > Does this make sense for a passed in fd? If someone passes a "server"
> > fd, are they likely to be passing a socket on which bind() but not
> > listen() has been called? Or one on which both bind() and listen()
> > have been called?
> >
>
> Original code in net/socket.c doesn't manage server case with fd.
>
> So I have checked what is done for QIO (all this code is overwritten by
> patch introducing QIO anyway):
>
> At the end of the series, we use qio_channel_socket_listen_async() in
> net_stream_server_init(), that in the end calls socket_listen().
>
> With SOCKET_ADDRESS_TYPE_FD we does the listen() (without bind()) with the
> following comment:
>
> case SOCKET_ADDRESS_TYPE_FD:
> fd = socket_get_fd(addr->u.fd.str, errp);
> if (fd < 0) {
> return -1;
> }
>
> /*
> * If the socket is not yet in the listen state, then transition it to
> * the listen state now.
> *
> * If it's already listening then this updates the backlog value as
> * requested.
> *
> * If this socket cannot listen because it's already in another state
> * (e.g. unbound or connected) then we'll catch the error here.
> */
> if (listen(fd, num) != 0) {
> error_setg_errno(errp, errno, "Failed to listen on fd socket");
> closesocket(fd);
> return -1;
> }
> break;
>
> So I think we should keep the listen() in our case too.
Ok, that makes sense to me. Or at least, if it's not correct we
should fix it later for all the places at the same time in the qio
code.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH v9 05/16] qapi: net: add stream and dgram netdevs
On 9/28/22 07:55, David Gibson wrote:
+static int net_stream_server_init(NetClientState *peer,
+ const char *model,
+ const char *name,
+ SocketAddress *addr,
+ Error **errp)
+{
+NetClientState *nc;
+NetStreamState *s;
+int fd, ret;
+
+switch (addr->type) {
+case SOCKET_ADDRESS_TYPE_INET: {
+struct sockaddr_in saddr_in;
+
+if (convert_host_port(_in, addr->u.inet.host, addr->u.inet.port,
+ errp) < 0) {
+return -1;
+}
+
+fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
+if (fd < 0) {
+error_setg_errno(errp, errno, "can't create stream socket");
+return -1;
+}
+qemu_socket_set_nonblock(fd);
+
+socket_set_fast_reuse(fd);
+
+ret = bind(fd, (struct sockaddr *)_in, sizeof(saddr_in));
+if (ret < 0) {
+error_setg_errno(errp, errno, "can't bind ip=%s to socket",
+ inet_ntoa(saddr_in.sin_addr));
+closesocket(fd);
+return -1;
+}
+break;
+}
+case SOCKET_ADDRESS_TYPE_FD:
+fd = monitor_fd_param(monitor_cur(), addr->u.fd.str, errp);
+if (fd == -1) {
+return -1;
+}
+ret = qemu_socket_try_set_nonblock(fd);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "%s: Can't use file descriptor %d",
+ name, fd);
+return -1;
+}
+break;
+default:
+error_setg(errp, "only support inet or fd type");
+return -1;
+}
+
+ret = listen(fd, 0);
Does this make sense for a passed in fd? If someone passes a "server"
fd, are they likely to be passing a socket on which bind() but not
listen() has been called? Or one on which both bind() and listen()
have been called?
Original code in net/socket.c doesn't manage server case with fd.
So I have checked what is done for QIO (all this code is overwritten by patch introducing
QIO anyway):
At the end of the series, we use qio_channel_socket_listen_async() in
net_stream_server_init(), that in the end calls socket_listen().
With SOCKET_ADDRESS_TYPE_FD we does the listen() (without bind()) with the
following comment:
case SOCKET_ADDRESS_TYPE_FD:
fd = socket_get_fd(addr->u.fd.str, errp);
if (fd < 0) {
return -1;
}
/*
* If the socket is not yet in the listen state, then transition it to
* the listen state now.
*
* If it's already listening then this updates the backlog value as
* requested.
*
* If this socket cannot listen because it's already in another state
* (e.g. unbound or connected) then we'll catch the error here.
*/
if (listen(fd, num) != 0) {
error_setg_errno(errp, errno, "Failed to listen on fd socket");
closesocket(fd);
return -1;
}
break;
So I think we should keep the listen() in our case too.
Thanks,
Laurent
Re: [PATCH v9 05/16] qapi: net: add stream and dgram netdevs
On Mon, Sep 26, 2022 at 09:50:37PM +0200, Laurent Vivier wrote:
> Copied from socket netdev file and modified to use SocketAddress
> to be able to introduce new features like unix socket.
>
> "udp" and "mcast" are squashed into dgram netdev, multicast is detected
> according to the IP address type.
> "listen" and "connect" modes are managed by stream netdev. An optional
> parameter "server" defines the mode (server by default)
>
> The two new types need to be parsed the modern way with -netdev, because
> with the traditional way, the "type" field of netdev structure collides with
> the "type" field of SocketAddress and prevents the correct evaluation of the
> command line option. Moreover the traditional way doesn't allow to use
> the same type (SocketAddress) several times with the -netdev option
> (needed to specify "local" and "remote" addresses).
>
> The previous commit paved the way for parsing the modern way, but
> omitted one detail: how to pick modern vs. traditional, in
> netdev_is_modern().
>
> We want to pick based on the value of parameter "type". But how to
> extract it from the option argument?
>
> Parsing the option argument, either the modern or the traditional way,
> extracts it for us, but only if parsing succeeds.
>
> If parsing fails, there is no good option. No matter which parser we
> pick, it'll be the wrong one for some arguments, and the error
> reporting will be confusing.
>
> Fortunately, the traditional parser accepts *anything* when called in
> a certain way. This maximizes our chance to extract the value of
> "type", and in turn minimizes the risk of confusing error reporting.
>
> Signed-off-by: Laurent Vivier
> Reviewed-by: Stefano Brivio
Mostly LGTM, but a few minor points noted below.
> ---
> hmp-commands.hx | 2 +-
> net/clients.h | 6 +
> net/dgram.c | 542
> net/hub.c | 2 +
> net/meson.build | 2 +
> net/net.c | 30 ++-
> net/stream.c| 423 +
> qapi/net.json | 63 +-
> qemu-options.hx | 12 ++
> 9 files changed, 1078 insertions(+), 4 deletions(-)
> create mode 100644 net/dgram.c
> create mode 100644 net/stream.c
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8ab8000acd9e..da40a7eb04ed 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1276,7 +1276,7 @@ ERST
> {
> .name = "netdev_add",
> .args_type = "netdev:O",
> -.params = "[user|tap|socket|vde|bridge|hubport|netmap|vhost-user"
> +.params =
> "[user|tap|socket|stream|dgram|vde|bridge|hubport|netmap|vhost-user"
> #ifdef CONFIG_VMNET
>"|vmnet-host|vmnet-shared|vmnet-bridged"
> #endif
> diff --git a/net/clients.h b/net/clients.h
> index c9157789f2ce..ed8bdfff1e7c 100644
> --- a/net/clients.h
> +++ b/net/clients.h
> @@ -40,6 +40,12 @@ int net_init_hubport(const Netdev *netdev, const char
> *name,
> int net_init_socket(const Netdev *netdev, const char *name,
> NetClientState *peer, Error **errp);
>
> +int net_init_stream(const Netdev *netdev, const char *name,
> +NetClientState *peer, Error **errp);
> +
> +int net_init_dgram(const Netdev *netdev, const char *name,
> + NetClientState *peer, Error **errp);
> +
> int net_init_tap(const Netdev *netdev, const char *name,
> NetClientState *peer, Error **errp);
>
> diff --git a/net/dgram.c b/net/dgram.c
> new file mode 100644
> index ..45d869efc844
> --- /dev/null
> +++ b/net/dgram.c
> @@ -0,0 +1,542 @@
> +/*
> + * QEMU System Emulator
> + *
> + * Copyright (c) 2003-2008 Fabrice Bellard
I see in this spin you added your (well, Red Hat's) copyright to
stream.c, but not to this one.
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> + * of this software and associated documentation files (the "Software"), to
> deal
> + * in the Software without restriction, including without limitation the
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +
>
Re: [PATCH v9 05/16] qapi: net: add stream and dgram netdevs
Laurent Vivier writes:
> Copied from socket netdev file and modified to use SocketAddress
> to be able to introduce new features like unix socket.
>
> "udp" and "mcast" are squashed into dgram netdev, multicast is detected
> according to the IP address type.
> "listen" and "connect" modes are managed by stream netdev. An optional
> parameter "server" defines the mode (server by default)
>
> The two new types need to be parsed the modern way with -netdev, because
> with the traditional way, the "type" field of netdev structure collides with
> the "type" field of SocketAddress and prevents the correct evaluation of the
> command line option. Moreover the traditional way doesn't allow to use
> the same type (SocketAddress) several times with the -netdev option
> (needed to specify "local" and "remote" addresses).
>
> The previous commit paved the way for parsing the modern way, but
> omitted one detail: how to pick modern vs. traditional, in
> netdev_is_modern().
>
> We want to pick based on the value of parameter "type". But how to
> extract it from the option argument?
>
> Parsing the option argument, either the modern or the traditional way,
> extracts it for us, but only if parsing succeeds.
>
> If parsing fails, there is no good option. No matter which parser we
> pick, it'll be the wrong one for some arguments, and the error
> reporting will be confusing.
>
> Fortunately, the traditional parser accepts *anything* when called in
> a certain way. This maximizes our chance to extract the value of
> "type", and in turn minimizes the risk of confusing error reporting.
>
> Signed-off-by: Laurent Vivier
> Reviewed-by: Stefano Brivio
> ---
> hmp-commands.hx | 2 +-
> net/clients.h | 6 +
> net/dgram.c | 542
> net/hub.c | 2 +
> net/meson.build | 2 +
> net/net.c | 30 ++-
> net/stream.c| 423 +
> qapi/net.json | 63 +-
> qemu-options.hx | 12 ++
> 9 files changed, 1078 insertions(+), 4 deletions(-)
> create mode 100644 net/dgram.c
> create mode 100644 net/stream.c
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8ab8000acd9e..da40a7eb04ed 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1276,7 +1276,7 @@ ERST
> {
> .name = "netdev_add",
> .args_type = "netdev:O",
> -.params = "[user|tap|socket|vde|bridge|hubport|netmap|vhost-user"
> +.params =
> "[user|tap|socket|stream|dgram|vde|bridge|hubport|netmap|vhost-user"
> #ifdef CONFIG_VMNET
>"|vmnet-host|vmnet-shared|vmnet-bridged"
> #endif
> diff --git a/net/clients.h b/net/clients.h
[...]
> diff --git a/qapi/net.json b/qapi/net.json
> index dd088c09c509..e02e8001a000 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -7,6 +7,7 @@
> ##
>
> { 'include': 'common.json' }
> +{ 'include': 'sockets.json' }
>
> ##
> # @set_link:
> @@ -573,6 +574,61 @@
> '*isolated': 'bool' },
>'if': 'CONFIG_VMNET' }
>
> +##
> +# @NetdevStreamOptions:
> +#
> +# Configuration info for stream socket netdev
> +#
> +# @addr: socket address to listen on (server=true)
> +#or connect to (server=false)
> +# @server: create server socket (default: true)
> +#
> +# Only SocketAddress types 'inet' and 'fd' are supported.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'NetdevStreamOptions',
> + 'data': {
> +'addr': 'SocketAddress',
> +'*server': 'bool' } }
> +
> +##
> +# @NetdevDgramOptions:
> +#
> +# Configuration info for datagram socket netdev.
> +#
> +# @remote: remote address
> +# @local: local address
> +#
> +# Only SocketAddress types 'inet' and 'fd' are supported.
> +#
> +# The code checks there is at least one of these options and reports an error
> +# if not.
Can we drop this sentence?
>If remote address is present and it's a multicast address, local
> +# address is optional. Otherwise local address is required and remote address
> +# is optional.
> +#
> +# .. table:: Valid parameters combination table
> +#:widths: auto
> +#
> +#= =
> +#remote local okay?
> +#= =
> +#absent absentno
> +#absent not fdno
> +#absent fdyes
> +#multicast absentyes
> +#multicast present yes
> +#not multicast absentno
> +#not multicast present yes
> +#= =
> +#
> +# Since: 7.1
> +##
My networking fu is not strong enough to suggest further improvements.
So let's go with what we have here.
> +{ 'struct': 'NetdevDgramOptions',
> + 'data': {
> +'*local': 'SocketAddress',
> +'*remote': 'SocketAddress' } }
> +
> ##
> # @NetClientDriver:
> #
> @@ -586,8 +642,9 @@
> #@vmnet-bridged since 7.1
> ##
> { 'enum': 'NetClientDriver',
> - 'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
> -
[PATCH v9 05/16] qapi: net: add stream and dgram netdevs
Copied from socket netdev file and modified to use SocketAddress
to be able to introduce new features like unix socket.
"udp" and "mcast" are squashed into dgram netdev, multicast is detected
according to the IP address type.
"listen" and "connect" modes are managed by stream netdev. An optional
parameter "server" defines the mode (server by default)
The two new types need to be parsed the modern way with -netdev, because
with the traditional way, the "type" field of netdev structure collides with
the "type" field of SocketAddress and prevents the correct evaluation of the
command line option. Moreover the traditional way doesn't allow to use
the same type (SocketAddress) several times with the -netdev option
(needed to specify "local" and "remote" addresses).
The previous commit paved the way for parsing the modern way, but
omitted one detail: how to pick modern vs. traditional, in
netdev_is_modern().
We want to pick based on the value of parameter "type". But how to
extract it from the option argument?
Parsing the option argument, either the modern or the traditional way,
extracts it for us, but only if parsing succeeds.
If parsing fails, there is no good option. No matter which parser we
pick, it'll be the wrong one for some arguments, and the error
reporting will be confusing.
Fortunately, the traditional parser accepts *anything* when called in
a certain way. This maximizes our chance to extract the value of
"type", and in turn minimizes the risk of confusing error reporting.
Signed-off-by: Laurent Vivier
Reviewed-by: Stefano Brivio
---
hmp-commands.hx | 2 +-
net/clients.h | 6 +
net/dgram.c | 542
net/hub.c | 2 +
net/meson.build | 2 +
net/net.c | 30 ++-
net/stream.c| 423 +
qapi/net.json | 63 +-
qemu-options.hx | 12 ++
9 files changed, 1078 insertions(+), 4 deletions(-)
create mode 100644 net/dgram.c
create mode 100644 net/stream.c
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8ab8000acd9e..da40a7eb04ed 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1276,7 +1276,7 @@ ERST
{
.name = "netdev_add",
.args_type = "netdev:O",
-.params = "[user|tap|socket|vde|bridge|hubport|netmap|vhost-user"
+.params =
"[user|tap|socket|stream|dgram|vde|bridge|hubport|netmap|vhost-user"
#ifdef CONFIG_VMNET
"|vmnet-host|vmnet-shared|vmnet-bridged"
#endif
diff --git a/net/clients.h b/net/clients.h
index c9157789f2ce..ed8bdfff1e7c 100644
--- a/net/clients.h
+++ b/net/clients.h
@@ -40,6 +40,12 @@ int net_init_hubport(const Netdev *netdev, const char *name,
int net_init_socket(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp);
+int net_init_stream(const Netdev *netdev, const char *name,
+NetClientState *peer, Error **errp);
+
+int net_init_dgram(const Netdev *netdev, const char *name,
+ NetClientState *peer, Error **errp);
+
int net_init_tap(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp);
diff --git a/net/dgram.c b/net/dgram.c
new file mode 100644
index ..45d869efc844
--- /dev/null
+++ b/net/dgram.c
@@ -0,0 +1,542 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+
+#include "net/net.h"
+#include "clients.h"
+#include "monitor/monitor.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qemu/option.h"
+#include "qemu/sockets.h"
+#include "qemu/iov.h"
+#include "qemu/main-loop.h"
+#include "qemu/cutils.h"
+
+typedef struct NetDgramState {
+NetClientState nc;
+int fd;
+SocketReadState rs;
+struct sockaddr_in dgram_dst; /* contains destination iff connectionless */
+bool
