Re: [PATCH v3 1/4] tests: Add tests for yank with the chardev-change case

[Lukas Straub]

On Fri, 26 Mar 2021 00:36:45 +0400
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Tue, Mar 23, 2021 at 9:57 PM Lukas Straub <lukasstraub2@web.de> wrote:
> 
> > Add tests for yank with the chardev-change case.
> >
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >  MAINTAINERS            |   1 +
> >  tests/unit/meson.build |   3 +-
> >  tests/unit/test-yank.c | 201 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 204 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/unit/test-yank.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 77259c031d..accb683a55 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2821,6 +2821,7 @@ M: Lukas Straub <lukasstraub2@web.de>
> >  S: Odd fixes
> >  F: util/yank.c
> >  F: migration/yank_functions*
> > +F: tests/unit/test-yank.c
> >  F: include/qemu/yank.h
> >  F: qapi/yank.json
> >
> > diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> > index 4bfe4627ba..b3bc2109da 100644
> > --- a/tests/unit/meson.build
> > +++ b/tests/unit/meson.build
> > @@ -123,7 +123,8 @@ if have_system
> >      'test-util-sockets': ['socket-helpers.c'],
> >      'test-base64': [],
> >      'test-bufferiszero': [],
> > -    'test-vmstate': [migration, io]
> > +    'test-vmstate': [migration, io],
> > +    'test-yank': ['socket-helpers.c', qom, io, chardev]
> >    }
> >    if 'CONFIG_INOTIFY1' in config_host
> >      tests += {'test-util-filemonitor': []}
> > diff --git a/tests/unit/test-yank.c b/tests/unit/test-yank.c
> > new file mode 100644
> > index 0000000000..5cb94c2fe4
> > --- /dev/null
> > +++ b/tests/unit/test-yank.c
> > @@ -0,0 +1,201 @@
> > +/*
> > + * Tests for QEMU yank feature
> > + *
> > + * Copyright (c) Lukas Straub <lukasstraub2@web.de>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include <glib/gstdio.h>
> > +
> > +#include "qemu/config-file.h"
> > +#include "qemu/module.h"
> > +#include "qemu/option.h"
> > +#include "chardev/char-fe.h"
> > +#include "sysemu/sysemu.h"
> > +#include "qapi/error.h"
> > +#include "qapi/qapi-commands-char.h"
> > +#include "qapi/qapi-types-char.h"
> > +#include "qapi/qapi-commands-yank.h"
> > +#include "qapi/qapi-types-yank.h"
> > +#include "io/channel-socket.h"
> > +#include "socket-helpers.h"
> > +
> > +typedef struct {
> > +    SocketAddress *addr;
> > +    bool old_yank;
> > +    bool new_yank;
> > +    bool fail;
> > +} CharChangeTestConfig;
> > +
> > +static int chardev_change(void *opaque)
> > +{
> > +    return 0;
> > +}
> > +
> > +static bool is_yank_instance_registered(void)
> > +{
> > +    YankInstanceList *list;
> > +    bool ret;
> > +
> > +    list = qmp_query_yank(&error_abort);
> > +
> > +    ret = !!list;
> > +
> > +    qapi_free_YankInstanceList(list);
> > +
> > +    return ret;
> > +}
> > +
> > +static void char_change_test(gconstpointer opaque)
> > +{
> > +    CharChangeTestConfig *conf = (gpointer) opaque;
> > +    SocketAddress *addr;
> > +    Chardev *chr;
> > +    CharBackend be;
> > +    ChardevReturn *ret;
> > +    QIOChannelSocket *ioc;
> > +
> > +    /*
> > +     * Setup a listener socket and determine its address
> > +     * so we know the TCP port for the client later
> > +     */
> > +    ioc = qio_channel_socket_new();
> > +    g_assert_nonnull(ioc);
> > +    qio_channel_socket_listen_sync(ioc, conf->addr, 1, &error_abort);
> > +    addr = qio_channel_socket_get_local_address(ioc, &error_abort);
> > +    g_assert_nonnull(addr);
> > +
> > +    ChardevBackend backend[2] = {
> > +        /* doesn't support yank */
> > +        { .type = CHARDEV_BACKEND_KIND_NULL },
> > +        /* supports yank */
> > +        {
> > +            .type = CHARDEV_BACKEND_KIND_SOCKET,
> > +            .u.socket.data = &(ChardevSocket) {
> > +                .addr = &(SocketAddressLegacy) {
> > +                    .type = SOCKET_ADDRESS_LEGACY_KIND_INET,
> > +                    .u.inet.data = &addr->u.inet
> > +                },
> > +                .has_server = true,
> > +                .server = false
> > +            }
> > +        } };
> > +
> > +    ChardevBackend fail_backend[2] = {
> > +        /* doesn't support yank */
> > +        {
> > +            .type = CHARDEV_BACKEND_KIND_UDP,
> > +            .u.udp.data = &(ChardevUdp) {
> > +                .remote = &(SocketAddressLegacy) {
> > +                    .type = SOCKET_ADDRESS_LEGACY_KIND_UNIX,
> > +                    .u.q_unix.data = &(UnixSocketAddress) {
> > +                        .path = (char *)""
> > +                    }
> > +                }
> > +            }
> > +        },
> > +        /* supports yank */
> > +        {
> > +            .type = CHARDEV_BACKEND_KIND_SOCKET,
> > +            .u.socket.data = &(ChardevSocket) {
> > +                .addr = &(SocketAddressLegacy) {
> > +                    .type = SOCKET_ADDRESS_LEGACY_KIND_INET,
> > +                    .u.inet.data = &(InetSocketAddress) {
> > +                        .host = (char *)"127.0.0.1",
> > +                        .port = (char *)"0"
> > +                    }
> > +                },
> > +                .has_server = true,
> > +                .server = false
> > +            }
> > +        } };
> > +
> > +    g_assert(!is_yank_instance_registered());
> > +
> > +    ret = qmp_chardev_add("chardev", &backend[conf->old_yank],
> > &error_abort);
> > +    qapi_free_ChardevReturn(ret);
> > +    chr = qemu_chr_find("chardev");
> > +    g_assert_nonnull(chr);
> > +
> > +    g_assert(is_yank_instance_registered() == conf->old_yank);
> > +
> > +    qemu_chr_wait_connected(chr, &error_abort);
> > +    qemu_chr_fe_init(&be, chr, &error_abort);
> > +    /* allow chardev-change */
> > +    qemu_chr_fe_set_handlers(&be, NULL, NULL,
> > +                             NULL, chardev_change, NULL, NULL, true);
> > +
> > +    if (conf->fail) {
> > +        g_setenv("QTEST_SILENT_ERRORS", "1", 1);
> > +        ret = qmp_chardev_change("chardev", &fail_backend[conf->new_yank],
> > +                                 NULL);
> > +        g_assert_null(ret);
> > +        g_assert(be.chr == chr);
> > +        g_assert(is_yank_instance_registered() == conf->old_yank);
> > +        g_unsetenv("QTEST_SILENT_ERRORS");
> > +    } else {
> > +        ret = qmp_chardev_change("chardev", &backend[conf->new_yank],
> > +                                 &error_abort);
> > +        g_assert_nonnull(ret);
> > +        g_assert(be.chr != chr);
> > +        g_assert(is_yank_instance_registered() == conf->new_yank);
> > +    }
> > +
> > +    object_unparent(OBJECT(be.chr));
> > +    object_unref(OBJECT(ioc));
> > +    qapi_free_ChardevReturn(ret);
> > +}
> > +
> > +static SocketAddress tcpaddr = {
> > +    .type = SOCKET_ADDRESS_TYPE_INET,
> > +    .u.inet.host = (char *)"127.0.0.1",
> > +    .u.inet.port = (char *)"0",
> > +};
> > +
> > +int main(int argc, char **argv)
> > +{
> > +    bool has_ipv4, has_ipv6;
> > +
> > +    qemu_init_main_loop(&error_abort);
> > +    socket_init();
> > +
> > +    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 (!has_ipv4) {
> > +        goto end;
> > +    }
> > +
> > +    module_call_init(MODULE_INIT_QOM);
> > +    qemu_add_opts(&qemu_chardev_opts);
> > +
> > +#define CHAR_CHANGE_TEST(name, _old_yank, _new_yank)
> >      \
> > +    do {
> >      \
> > +        g_test_add_data_func("/yank/char_change/success/" # name,
> >       \
> > +                             &(CharChangeTestConfig) { .addr = &tcpaddr,
> >      \
> > +                                                       .old_yank =
> > (_old_yank),\
> > +                                                       .new_yank =
> > (_new_yank),\
> > +                                                       .fail = false },
> >       \
> > +                             char_change_test);
> >       \
> > +        g_test_add_data_func("/yank/char_change/fail/" # name,
> >      \
> > +                             &(CharChangeTestConfig) { .addr = &tcpaddr,
> >      \
> > +                                                       .old_yank =
> > (_old_yank),\
> > +                                                       .new_yank =
> > (_new_yank),\
> > +                                                       .fail = true },
> >      \
> > +                             char_change_test);
> >       \
> > +    } while (0)
> > +
> >
> 
> If you run the test under ASAN, you get an error. Config is
> stack-use-after-scope.

Dammit, the do { } while(0) was a last-minute addition because checkpatch.pl
complained about it. I'll fix it.

> Otherwise, the test looks good to me, but please introduce the test after
> the fix in the series, to avoid intermittent breaking when doing git
> rebase. Iow, we make our best to have no regressions introduced in a final
> series, even temporarily in tests.

Regards,
Lukas Straub

> thanks
> 
> 
> 
> > +    CHAR_CHANGE_TEST(to_yank, false, true);
> > +    CHAR_CHANGE_TEST(yank_to_yank, true, true);
> > +    CHAR_CHANGE_TEST(from_yank, true, false);
> > +
> > +end:
> > +    return g_test_run();
> > +}
> > --
> > 2.30.2
> >
> >
> 



--

pgp03Yao0Soum.pgp
Description: OpenPGP digital signature