[libvirt] [PATCH v3] Loop through all resolved addresses in virNetSocketNewListenTCP

[Daniel P. Berrangé]

On Mon, Jun 04, 2018 at 12:29:37PM +0200, Olaf Hering wrote:
> Currently virNetSocketNewListenTCP bails out early under the following
> conditions:
> - the hostname resolves to at least one IPv4 and at least one IPv6
>   address
> - the local interfaces have that one IPv4 address assigned, but not any
>   of the IPv6 addresses
> - the local interfaces have just IPv6 link-local addresses
> 
> In this case the resolver returns not only the IPv4 addresses but also
> IPv6. Binding the IPv6 address will obviously fail. But this terminates
> the entire loop, even if binding to IPv4 succeeded.

Could you elaborate on this a bit more, as on reflection I'm not sure
I understand the flaw here. Could you show the 'ip addr' output and
corresponding hostname resolution results, and say what errno you
are seeing from bind().

> 
> To fix this error, just keep going and loop through all returned
> addresses. In case none of the attempts to bind to some address
> succeeded, try to report some appropriate error.
> 
> Signed-off-by: Olaf Hering <olaf at aepfle.de>
> ---
> 
> v3:
>  more whitespace fixes, as suggested by Daniel P. Berrangé
> v2:
>  whitespace fixes, as suggested by John Ferlan
> 
>  src/rpc/virnetsocket.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index 7087abec9c..3a055acd39 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -382,11 +382,8 @@ int virNetSocketNewListenTCP(const char *nodename,
>  #endif
>  
>          if (bind(fd, runp->ai_addr, runp->ai_addrlen) < 0) {
> -            if (errno != EADDRINUSE) {
> -                virReportSystemError(errno, "%s", _("Unable to bind to port"));
> -                goto error;
> -            }
> -            addrInUse = true;
> +            if (errno == EADDRINUSE)
> +                addrInUse = true;
>              VIR_FORCE_CLOSE(fd);
>              runp = runp->ai_next;
>              continue;
> @@ -409,14 +406,14 @@ int virNetSocketNewListenTCP(const char *nodename,
>          fd = -1;
>      }
>  
> -    if (nsocks == 0 && familyNotSupported) {
> -        virReportSystemError(EAFNOSUPPORT, "%s", _("Unable to bind to port"));
> -        goto error;
> -    }
> -
> -    if (nsocks == 0 &&
> -        addrInUse) {
> -        virReportSystemError(EADDRINUSE, "%s", _("Unable to bind to port"));
> +    if (nsocks == 0) {
> +        if (familyNotSupported)
> +            errno = EAFNOSUPPORT;
> +        else if (addrInUse)
> +            errno = EADDRINUSE;
> +        else
> +            errno = EDESTADDRREQ;

This EDESTADDRREQ feels a bit odd to use - "Destination address required"
doesn't make much sense for something that has to be a local address.

> +        virReportSystemError(errno, "%s", _("Unable to bind to port"));
>          goto error;
>      }
>  

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