Re: [PATCH v3] tests/qtest: netdev: test stream and dgram backends

[Thomas Huth <thuth@redhat.com>]

On 09/11/2022 14.03, Laurent Vivier wrote:
[...]
> diff --git a/tests/qtest/netdev-socket.c b/tests/qtest/netdev-socket.c
> new file mode 100644
> index 000000000000..b6b59244a282
> --- /dev/null
> +++ b/tests/qtest/netdev-socket.c
> @@ -0,0 +1,435 @@
> +/*
> + * QTest testcase for netdev stream and dgram
> + *
> + * Copyright (c) 2022 Red Hat, Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include <glib/gstdio.h>
> +#include "../unit/socket-helpers.h"
> +#include "libqtest.h"
> +
> +#define CONNECTION_TIMEOUT    5
> +
> +#define EXPECT_STATE(q, e, t)                             \
> +do {                                                      \
> +    char *resp = qtest_hmp(q, "info network");            \
> +    if (t) {                                              \
> +        strrchr(resp, t)[0] = 0;                          \
> +    }                                                     \
> +    g_test_timer_start();                                 \
> +    while (g_test_timer_elapsed() < CONNECTION_TIMEOUT) { \
> +        if (strcmp(resp, e) == 0) {                       \
> +            break;                                        \
> +        }                                                 \
> +        g_free(resp);                                     \
> +        resp = qtest_hmp(q, "info network");              \
> +        if (t) {                                          \
> +            strrchr(resp, t)[0] = 0;                      \
> +        }                                                 \
> +    }                                                     \
> +    g_assert_cmpstr(resp, ==, e);                         \
> +    g_free(resp);                                         \
> +} while (0)

Wouldn't it be possible to write this without the duplicated qtest_hmp() ? 
Something like this:

#define EXPECT_STATE(q, e, t)                             \
do {                                                      \
     char *resp = NULL;                                    \
     g_test_timer_start();                                 \
     do {                                                  \
         g_free(resp);                                     \
         resp = qtest_hmp(q, "info network");              \
         if (t) {                                          \
             strrchr(resp, t)[0] = 0;                      \
         }                                                 \
         if (g_strequal(resp, e)) {                        \
             break;                                        \
         }                                                 \
     } while (g_test_timer_elapsed() < CONNECTION_TIMEOUT); \
     g_assert_cmpstr(resp, ==, e);                         \
     g_free(resp);                                         \
} while (0)

?

Also matching strings against the output of a HMP command sound very fragile 
- isn't there a way to do this with QMP instead?

[...]
> +int main(int argc, char **argv)
> +{
> +    int ret;
> +    bool has_ipv4, has_ipv6, has_afunix;
> +    gchar dir[] = "/tmp/netdev-socket.XXXXXX";

No more hard-coded /tmp/ paths, please. We're currently in progress of 
enabling the qtests on Windows, too. Please use g_dir_make_tmp() or 
something similar instead.

> +    g_test_init(&argc, &argv, NULL);
> +
> +    if (socket_check_protocol_support(&has_ipv4, &has_ipv6) < 0) {
> +        g_printerr("socket_check_protocol_support() failed\n");
> +        goto end;
> +    }
> +
> +    if (g_mkdtemp(dir) == NULL) {
> +        g_error("g_mkdtemp: %s", g_strerror(errno));
> +    }
> +    tmpdir = dir;
> +
> +    if (has_ipv4) {
> +        qtest_add_func("/netdev/stream/inet/ipv4", test_stream_inet_ipv4);
> +        qtest_add_func("/netdev/dgram/inet", test_dgram_inet);
> +        qtest_add_func("/netdev/dgram/mcast", test_dgram_mcast);
> +    }
> +    if (has_ipv6) {
> +        qtest_add_func("/netdev/stream/inet/ipv6", test_stream_inet_ipv6);
> +    }
> +
> +    socket_check_afunix_support(&has_afunix);
> +    if (has_afunix) {
> +        qtest_add_func("/netdev/dgram/unix", test_dgram_unix);
> +        qtest_add_func("/netdev/stream/unix", test_stream_unix);
> +        qtest_add_func("/netdev/stream/unix/abstract",
> +                       test_stream_unix_abstract);
> +        qtest_add_func("/netdev/stream/fd", test_stream_fd);
> +        qtest_add_func("/netdev/dgram/fd", test_dgram_fd);
> +    }
> +
> +end:
> +    ret = g_test_run();
> +
> +    g_rmdir(dir);

Maybe check the return code of g_rmdir(), to make sure that all temporary 
files in the directory have indeed been cleaned up successfully?

> +    return ret;
> +}

  Thomas