| Message ID | 20170926125952.7601-1-jfreimann@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: fix check for number of parameters to -netdev socket | expand |
Hi, Jens On 09/26/2017 08:59 PM, Jens Freimann wrote: > Since commit 0f8c289ad "net: fix -netdev socket,fd= for UDP sockets" > we allow more than one parameter for -netdev socket. But now > we run into an assert when no parameter at all is specified > >> qemu-system-x86_64 -net socket > socket.c:729: net_init_socket: Assertion `sock->has_udp' failed. > > Change the check and error message to reflect that at least > one of the options has to be specified. An example for when we need > more than on is when we hand in a UDP socket. Then we have fd= and udp= s/on/one > as parameters. > > Cc: Jason Wang <jasowang@redhat.com> > Fixes: 0f8c289ad539feb5135c545bea947b310a893f4b > Reported-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> > Signed-off-by: Jens Freimann <jfreimann@redhat.com> > --- > v1->v2: > - add check to prevent listen= and connect= being used at the same time > > net/socket.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/net/socket.c b/net/socket.c > index e6b471c63d..74a2eff2e0 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -695,13 +695,18 @@ int net_init_socket(const Netdev *netdev, const char *name, > assert(netdev->type == NET_CLIENT_DRIVER_SOCKET); > sock = &netdev->u.socket; > > - if (sock->has_listen + sock->has_connect + sock->has_mcast + > - sock->has_udp > 1) { > - error_setg(errp, "exactly one of listen=, connect=, mcast= or udp=" > + if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast + > + sock->has_udp == 0) { > + error_setg(errp, "at least one of fd=, listen=, connect=, mcast= or udp=" > " is required"); > return -1; > } Looks like this allow "listen" and "mcast" can be used at the same time? I'm not sure. the original: if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast + sock->has_udp != 1) It means that only one of these elements can be used to a QEmu instance at the same time, i.e these elements are mutually exclusive. After this patch, are some cases missing? It's just my personal understanding, please judge. :) Thanks, Mao > > + if (sock->has_listen && sock->has_connect) { > + error_setg(errp, "listen= and connect= can't be used together"); > + return -1; > + } > + > if (sock->has_localaddr && !sock->has_mcast && !sock->has_udp) { > error_setg(errp, "localaddr= is only valid with mcast= or udp="); > return -1; >
On Tue, Sep 26, 2017 at 02:38:33PM +0000, Mao Zhongyi wrote: > >Hi, Jens > > >On 09/26/2017 08:59 PM, Jens Freimann wrote: >>Since commit 0f8c289ad "net: fix -netdev socket,fd= for UDP sockets" >>we allow more than one parameter for -netdev socket. But now >>we run into an assert when no parameter at all is specified >> >>>qemu-system-x86_64 -net socket >>socket.c:729: net_init_socket: Assertion `sock->has_udp' failed. >> >>Change the check and error message to reflect that at least >>one of the options has to be specified. An example for when we need >>more than on is when we hand in a UDP socket. Then we have fd= and udp= > >s/on/one > >>as parameters. >> >>Cc: Jason Wang <jasowang@redhat.com> >>Fixes: 0f8c289ad539feb5135c545bea947b310a893f4b >>Reported-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> >>Signed-off-by: Jens Freimann <jfreimann@redhat.com> >>--- >>v1->v2: >>- add check to prevent listen= and connect= being used at the same time >> >> net/socket.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >>diff --git a/net/socket.c b/net/socket.c >>index e6b471c63d..74a2eff2e0 100644 >>--- a/net/socket.c >>+++ b/net/socket.c >>@@ -695,13 +695,18 @@ int net_init_socket(const Netdev *netdev, const char *name, >> assert(netdev->type == NET_CLIENT_DRIVER_SOCKET); >> sock = &netdev->u.socket; >> >>- if (sock->has_listen + sock->has_connect + sock->has_mcast + >>- sock->has_udp > 1) { >>- error_setg(errp, "exactly one of listen=, connect=, mcast= or udp=" >>+ if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast + >>+ sock->has_udp == 0) { >>+ error_setg(errp, "at least one of fd=, listen=, connect=, mcast= or udp=" >> " is required"); >> return -1; >> } > >Looks like this allow "listen" and "mcast" can be used at the same time? >I'm not sure. > >the original: >if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast + > sock->has_udp != 1) > >It means that only one of these elements can be used to a QEmu instance at >the same time, i.e these elements are mutually exclusive. After this patch, >are some cases missing? > >It's just my personal understanding, please judge. :) Looking at this again and re-testing with the (not yet applied) pxe test for vhost-user bridge, I think we can just revert this check back to != 1. More than one parameters is not needed. I'll send a patch with only this change. Thanks for the review! regards, Jens
diff --git a/net/socket.c b/net/socket.c index e6b471c63d..74a2eff2e0 100644 --- a/net/socket.c +++ b/net/socket.c @@ -695,13 +695,18 @@ int net_init_socket(const Netdev *netdev, const char *name, assert(netdev->type == NET_CLIENT_DRIVER_SOCKET); sock = &netdev->u.socket; - if (sock->has_listen + sock->has_connect + sock->has_mcast + - sock->has_udp > 1) { - error_setg(errp, "exactly one of listen=, connect=, mcast= or udp=" + if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast + + sock->has_udp == 0) { + error_setg(errp, "at least one of fd=, listen=, connect=, mcast= or udp=" " is required"); return -1; } + if (sock->has_listen && sock->has_connect) { + error_setg(errp, "listen= and connect= can't be used together"); + return -1; + } + if (sock->has_localaddr && !sock->has_mcast && !sock->has_udp) { error_setg(errp, "localaddr= is only valid with mcast= or udp="); return -1;