Re: [Qemu-devel] [PATCH v4 2/4] sockets: factor out create_fast_reuse_socket

["Daniel P. Berrange" <berrange@redhat.com>]

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.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 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 :|