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

[Knut Omang]

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 <address@hidden>
> > ---
> >  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