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

[Knut Omang <knut.omang@oracle.com>]

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.

Thanks,
Knut

> >          }
> >  
> > -        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