Re: [Qemu-devel] [PATCH v5 3/9] sockets: pull code for testing IP availability out of specific test

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

On Mon, Mar 12, 2018 at 12:49:33PM +0000, Daniel P. Berrangé wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> The test-io-channel-socket.c file has some useful helper functions for
> checking if a specific IP protocol is available. Other tests need to
> perform similar kinds of checks to avoid running tests that will fail
> due to missing IP protocols.
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  tests/Makefile.include         |   2 +-
>  tests/socket-helpers.c         | 104 +++++++++++++++++++++++++++++++++++++++++
>  tests/socket-helpers.h         |  42 +++++++++++++++++
>  tests/test-io-channel-socket.c |  72 +---------------------------
>  4 files changed, 149 insertions(+), 71 deletions(-)
>  create mode 100644 tests/socket-helpers.c
>  create mode 100644 tests/socket-helpers.h
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index ef9b88c369..3d5b6174a8 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -715,7 +715,7 @@ tests/test-crypto-tlssession$(EXESUF): tests/test-crypto-tlssession.o \
>  	tests/crypto-tls-x509-helpers.o tests/pkix_asn1_tab.o $(test-crypto-obj-y)
>  tests/test-io-task$(EXESUF): tests/test-io-task.o $(test-io-obj-y)
>  tests/test-io-channel-socket$(EXESUF): tests/test-io-channel-socket.o \
> -        tests/io-channel-helpers.o $(test-io-obj-y)
> +        tests/io-channel-helpers.o tests/socket-helpers.o $(test-io-obj-y)
>  tests/tpm-crb-test$(EXESUF): tests/tpm-crb-test.o tests/tpm-emu.o $(test-io-obj-y)
>  tests/tpm-tis-test$(EXESUF): tests/tpm-tis-test.o tests/tpm-emu.o $(test-io-obj-y)
>  tests/test-io-channel-file$(EXESUF): tests/test-io-channel-file.o \
> diff --git a/tests/socket-helpers.c b/tests/socket-helpers.c
> new file mode 100644
> index 0000000000..13b6bb38eb
> --- /dev/null
> +++ b/tests/socket-helpers.c
> @@ -0,0 +1,104 @@
> +/*
> + * Helper functions for tests using sockets
> + *
> + * Copyright 2015-2018 Red Hat, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 or
> + * (at your option) version 3 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "qemu/sockets.h"
> +#include "socket-helpers.h"
> +
> +#ifndef AI_ADDRCONFIG
> +# define AI_ADDRCONFIG 0
> +#endif
> +#ifndef EAI_ADDRFAMILY
> +# define EAI_ADDRFAMILY 0
> +#endif
> +
> +int socket_can_bind(const char *hostname)
> +{
> +    int fd = -1;
> +    struct addrinfo ai, *res = NULL;
> +    int rc;
> +    int ret = -1;
> +
> +    memset(&ai, 0, sizeof(ai));
> +    ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
> +    ai.ai_family = AF_UNSPEC;
> +    ai.ai_socktype = SOCK_STREAM;
> +
> +    /* lookup */
> +    rc = getaddrinfo(hostname, NULL, &ai, &res);
> +    if (rc != 0) {
> +        if (rc == EAI_ADDRFAMILY ||
> +            rc == EAI_FAMILY) {
> +            errno = EADDRNOTAVAIL;
> +            goto done;
> +        }
> +        errno = EINVAL;
> +        goto cleanup;
> +    }
> +
> +    fd = qemu_socket(res->ai_family, res->ai_socktype, res->ai_protocol);
> +    if (fd < 0) {
> +        goto cleanup;
> +    }
> +
> +    if (bind(fd, res->ai_addr, res->ai_addrlen) < 0) {
> +        if (errno == EADDRNOTAVAIL) {
> +            goto done;
> +        }
> +        goto cleanup;
> +    }
> +
> + done:
> +    ret = 0;
> +
> + cleanup:
> +    if (fd != -1) {
> +        close(fd);
> +    }
> +    if (res) {
> +        freeaddrinfo(res);
> +    }
> +    return ret;
> +}
> +
> +
> +int socket_check_protocol_support(bool *has_ipv4, bool *has_ipv6)
> +{
> +    *has_ipv4 = *has_ipv6 = false;
> +
> +    if (socket_can_bind("127.0.0.1") < 0) {
> +        if (errno != EADDRNOTAVAIL) {
> +            return -1;
> +        }
> +    } else {
> +        *has_ipv4 = true;
> +    }
> +
> +    if (socket_can_bind("::1") < 0) {
> +        if (errno != EADDRNOTAVAIL) {
> +            return -1;
> +        }
> +    } else {
> +        *has_ipv6 = true;
> +    }
> +

Sigh, I should have kept the new code identical to the old code,
rather than trying to improve it, as this is in fact broken. The
socket_can_bind() is mistakenly returning '0' when EADDRNOTAVAIL
is set, so we always set the has_ipv4|6 vars to true.

It needs this squashed in:

@@ -48,9 +48,9 @@ int socket_can_bind(const char *hostname)
         if (rc == EAI_ADDRFAMILY ||
             rc == EAI_FAMILY) {
             errno = EADDRNOTAVAIL;
-            goto done;
+        } else {
+            errno = EINVAL;
         }
-        errno = EINVAL;
         goto cleanup;
     }
 
@@ -60,13 +60,9 @@ int socket_can_bind(const char *hostname)
     }
 
     if (bind(fd, res->ai_addr, res->ai_addrlen) < 0) {
-        if (errno == EADDRNOTAVAIL) {
-            goto done;
-        }
         goto cleanup;
     }
 
- done:
     ret = 0;
 
  cleanup:



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