On Mon, Jun 26, 2017 at 01:56:28PM +0200, Knut Omang wrote: > On Mon, 2017-06-26 at 11:28 +0100, Daniel P. Berrange wrote: > > On Fri, Jun 23, 2017 at 12:31:06PM +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.om...@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 852773d..699e36c 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"); > > > + } > > > > I think that having this method sometimes report an error message, and > > sometimes not report an error message, depending on state of a variable > > used by the caller is rather unpleasant. I'd much rather see this > > error message reporting remain in the caller. > > In principle I agree with you, but I think we do want to keep the details > of what the failure cause was by also propagating information about the > system > call that failed. > > I considered this an acceptable trade-off in the name of performance as well > as > readability at the next level. This is a fairly unlikely case that one really > does not > have to worry too much about at the next level. Setting an error that does > not get used > for that special, unlikely case is not that bad. Doing it for all failures > would be > a lot more unnecessary work. > > > > > > + 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; > > > > It isn't shown in this diff context, but at the end of the outer > > loop we have > > > > error_setg_errno(errp, errno, "Failed to find available port"); > > > > so IIUC, even this pre-existing code is wrong. If 'e->ai_next' is > > NULL, we report an error message here. Then, we continue to the > > next loop iteration, which causes use to terminate the loop > > entirely. At which point we'll report another error message > > over the top of the one we already have. > > > > So I think the error > > reporting does still need refactoring, but not in the way it > > is done here. > > I agree, a simple way to solve it would be to only set errp if no > error has already been set.
I'd like to see each of the methods set the error unconditionally. In the inet_socket_listen() method, we can use an then intermediate Error variable on each iteration of the loop, and propagate to the global variable at the end or discard it. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|