Re: [Qemu-devel] [PATCH v2 5/5] tests: add functional test validating ipv4/ipv6 address flag handling

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5479 bytes --]

On 05/19/2017 01:03 PM, Daniel P. Berrange wrote:
> The semantics around handling ipv4=on|off & ipv6=on|off are quite
> subtle to understand in combination with the various hostname addresses
> and backend types. Introduce a massive test matrix that launches QEMU
> and validates the ability to connect a client on each protocol as
> appropriate.
> 
> The test requires that the host has ability to bind to both :: and
> 0.0.0.0, on port 9000. If either protocol is not available, or if
> something is already listening on that port the test will skip.
> 
> Although it isn't using the QTest APIs, it expects the
> QTEST_QEMU_BINARY env variable to be set.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  tests/.gitignore           |   1 +

Nice - that often gets forgotten.

>  tests/Makefile.include     |   4 +
>  tests/test-sockets-proto.c | 855 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 860 insertions(+)
>  create mode 100644 tests/test-sockets-proto.c

'make check' passed for me with your patches (on a system with both IPv4
and IPv6 support), so:

Tested-by: Eric Blake <eblake@redhat.com>

I did not try what happens on an IPv4-only system, presumably the test
still behaves sanely there (where sanely may mean skipping rather than
completing?).

> +++ b/tests/test-sockets-proto.c
> @@ -0,0 +1,855 @@
> +/*
> + * QTest for IPv4/IPv6 protocol setup
> + *
> + * Copyright (c) 2017 Red Hat, Inc. and/or its affiliates

Interesting choice of attribution line.


> +/*
> + * This is the giant matrix of combinations we need to consider.
> + * There are 3 axes we deal with
> + *
> + * Axis 1: Protocol flags:
> + *
> + *  ipv4=unset, ipv6=unset  -> v4 & v6 clients ([1]
> + *  ipv4=unset, ipv6=off    -> v4 clients only
> + *  ipv4=unset, ipv6=on     -> v6 clients only
> + *  ipv4=off, ipv6=unset    -> v6 clients only
> + *  ipv4=off, ipv6=off      -> error - can't disable both [2]
> + *  ipv4=off, ipv6=on       -> v6 clients only
> + *  ipv4=on, ipv6=unset     -> v4 clients only
> + *  ipv4=on, ipv6=off       -> v4 clients only
> + *  ipv4=on, ipv6=on        -> v4 & v6 clients [3]
> + *
> + * Depending on the listening address, some of those combinations
> + * may result in errors. eg ipv4=off,ipv6=on combined with 0.0.0.0
> + * is nonsensical.
> + *
> + * [1] Some backends only support a single socket listener, so
> + *     will actually only allow v4 clients
> + * [2] QEMU should fail to startup in this case
> + * [3] If hostname is "" or "::", then we get a single listener
> + *     on IPv6 and thus can also accept v4 clients. For all other
> + *     hostnames, have same problem as [1].

Makes sense.

> + *
> + * Axis 2: Listening address:
> + *
> + *  ""        - resolves to 0.0.0.0 and ::, in that order
> + *  "0.0.0.0" - v4 clients only
> + *  "::"      - Mostly v6 clients only. Some scenarios should
> + *              permit v4 clients too.

Correct.

> + *
> + * Axis 3: Backend type:
> + *
> + *  Migration - restricted to a single listener. Also relies
> + *              on buggy inet_parse() which can't accept
> + *              =off/=on parameters to ipv4/ipv6 flags
> + *  Chardevs  - restricted to a single listener.
> + *  VNC       - supports multiple listeners. Also supports
> + *              socket ranges, so has extra set of tests
> + *              in the matrix

And explains the size of the test.  Thankfully it doesn't seem to add
too much noticeable additional time to 'make check'.

> + *
> + */
> +static QSocketsData test_data[] = {
> +    /* Migrate with "" address */
> +    /* XXX all settings with =off are disabled due to inet_parse() bug */
> +    /* XXX multilistener bug - should be .ipv6 = 1 */

Nice that you've pointed out spots for further improvements, and where
we EXPECT the test to change once we improve the code.


> +static pid_t run_qemu(const char *args)
> +{
> +    const char *pidfile = "test-sockets-proto.pid";
> +    char *pidstr;
> +    pid_t child;
> +    int status;
> +    pid_t ret;
> +    const char *binary = getenv("QTEST_QEMU_BINARY");
> +    long pid;
> +    if (binary == NULL) {
> +        g_printerr("Missing QTEST_QEMU_BINARY env variable");
> +    }
> +    g_assert(binary != NULL);

Should we do:

if (!binary) {
  message;
  exit(1);
}

instead of relying on g_assert() to do the exit?

> +    /* Now test IPv6 */
> +    child = run_qemu(data->args);
> +
> +    /*
> +     * The child should always succeed, because its the
> +     * same config as the succesful run we just did above

s/succesful/successful/

> +
> +int main(int argc, char **argv)
> +{
> +    int ret;
> +    gsize i;
> +
> +    if (check_protocol_support() < 0) {
> +        return 0; /* Skip test if we can't bind */

We don't have a magic number for skipped tests?  Many projects use exit
status 77 (rather than 0) to delineate a test that did not fail, but
whose skip results cannot be used as conclusive evidence of passing.  At
any rate, you've answered my question earlier - you do behave sanely if
you cannot test both IPv4 and IPv6 on a given host.

Typo is worth fixing, but minor enough that I'm comfortable giving:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]