[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 5/5] tests: add functional test validating ip
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 5/5] tests: add functional test validating ipv4/ipv6 address flag handling |
Date: |
Mon, 22 May 2017 11:00:26 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 |
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 <address@hidden>
> ---
> 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 <address@hidden>
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 <address@hidden>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v2 2/5] sockets: don't block IPv4 clients when listening on "::", (continued)