| Message ID | ee5cd7796873b061804bf6f8546759109473000b.1500709594.git-series.knut.omang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jul 22, 2017 at 09:49:31AM +0200, Knut Omang wrote: > First refactoring step to prepare for fixing the problem > exposed with the test-listen test in the previous commit > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > --- > util/qemu-sockets.c | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > index 1358c81..578f25b 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -149,6 +149,20 @@ int inet_ai_family_from_address(InetSocketAddress *addr, > return PF_UNSPEC; > } > > +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp) > +{ > + int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > + if (slisten < 0) { > + if (!e->ai_next) { > + error_setg_errno(errp, errno, "Failed to create socket"); > + } > + return -1; > + } > + > + socket_set_fast_reuse(slisten); > + return slisten; > +} As mentioned in the previous review, I don't think we should have methods like this which sometimes report an error on failure, and sometimes don't report an error. It makes it hard to review the callers for correctness of error handling. > + > static int inet_listen_saddr(InetSocketAddress *saddr, > int port_offset, > bool update_addr, > @@ -210,21 +224,17 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > return -1; > } > > - /* create socket + bind */ > + /* create socket + bind/listen */ > for (e = res; e != NULL; e = e->ai_next) { > getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, > uaddr,INET6_ADDRSTRLEN,uport,32, > NI_NUMERICHOST | NI_NUMERICSERV); > - slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > + > + slisten = create_fast_reuse_socket(e, &err); > if (slisten < 0) { > - if (!e->ai_next) { > - error_setg_errno(errp, errno, "Failed to create socket"); > - } So please leave this here. > continue; > } > > - socket_set_fast_reuse(slisten); > - > port_min = inet_getport(e); > port_max = saddr->has_to ? saddr->to + port_offset : port_min; > for (p = port_min; p <= port_max; p++) { Regards, Daniel
On Tue, 2017-07-25 at 10:38 +0100, Daniel P. Berrange wrote: > On Sat, Jul 22, 2017 at 09:49:31AM +0200, Knut Omang wrote: > > First refactoring step to prepare for fixing the problem > > exposed with the test-listen test in the previous commit > > > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > > --- > > util/qemu-sockets.c | 24 +++++++++++++++++------- > > 1 file changed, 17 insertions(+), 7 deletions(-) > > > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > > index 1358c81..578f25b 100644 > > --- a/util/qemu-sockets.c > > +++ b/util/qemu-sockets.c > > @@ -149,6 +149,20 @@ int inet_ai_family_from_address(InetSocketAddress *addr, > > return PF_UNSPEC; > > } > > > > +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp) > > +{ > > + int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > > + if (slisten < 0) { > > + if (!e->ai_next) { > > + error_setg_errno(errp, errno, "Failed to create socket"); > > + } > > + return -1; > > + } > > + > > + socket_set_fast_reuse(slisten); > > + return slisten; > > +} > > As mentioned in the previous review, I don't think we should have > methods like this which sometimes report an error on failure, and > sometimes don't report an error. It makes it hard to review the > callers for correctness of error handling. Sorry, somehow this one slipped through.. > > > + > > static int inet_listen_saddr(InetSocketAddress *saddr, > > int port_offset, > > bool update_addr, > > @@ -210,21 +224,17 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > > return -1; > > } > > > > - /* create socket + bind */ > > + /* create socket + bind/listen */ > > for (e = res; e != NULL; e = e->ai_next) { > > getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, > > uaddr,INET6_ADDRSTRLEN,uport,32, > > NI_NUMERICHOST | NI_NUMERICSERV); > > - slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > > + > > + slisten = create_fast_reuse_socket(e, &err); > > if (slisten < 0) { > > - if (!e->ai_next) { > > - error_setg_errno(errp, errno, "Failed to create socket"); > > - } > > So please leave this here. Since we are already outside of the scope of the original patch, let me suggest: As far as I can see there's no good reason to have different behavior if creating sockets on the last 'struct addrinfo' in the list fails than if an intermediate addrinfo fails and the next addrinfo in the list just happened to have no free ports. As far as I can see the 'Failed to create socket' message will be most useful to the user if creating a socket fails on all elements in the list, to detect issues with the network setup. I have implemented this semantics in v6 - it should be an enhancement while also simplifying the code inside the for loops, getting rid of the conditional error setting and avoid code duplication by handling the same error twice in proximity. Thanks, Knut > > > continue; > > } > > > > - socket_set_fast_reuse(slisten); > > - > > port_min = inet_getport(e); > > port_max = saddr->has_to ? saddr->to + port_offset : port_min; > > for (p = port_min; p <= port_max; p++) { > > > Regards, > Daniel
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 1358c81..578f25b 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -149,6 +149,20 @@ int inet_ai_family_from_address(InetSocketAddress *addr, return PF_UNSPEC; } +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp) +{ + int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); + if (slisten < 0) { + if (!e->ai_next) { + error_setg_errno(errp, errno, "Failed to create socket"); + } + return -1; + } + + socket_set_fast_reuse(slisten); + return slisten; +} + static int inet_listen_saddr(InetSocketAddress *saddr, int port_offset, bool update_addr, @@ -210,21 +224,17 @@ static int inet_listen_saddr(InetSocketAddress *saddr, return -1; } - /* create socket + bind */ + /* create socket + bind/listen */ for (e = res; e != NULL; e = e->ai_next) { getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, uaddr,INET6_ADDRSTRLEN,uport,32, NI_NUMERICHOST | NI_NUMERICSERV); - slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); + + slisten = create_fast_reuse_socket(e, &err); if (slisten < 0) { - if (!e->ai_next) { - error_setg_errno(errp, errno, "Failed to create socket"); - } continue; } - socket_set_fast_reuse(slisten); - port_min = inet_getport(e); port_max = saddr->has_to ? saddr->to + port_offset : port_min; for (p = port_min; p <= port_max; p++) {
First refactoring step to prepare for fixing the problem exposed with the test-listen test in the previous commit Signed-off-by: Knut Omang <knut.omang@oracle.com> --- util/qemu-sockets.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)