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

[Daniel P. Berrange]

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. 

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