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

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

--
2.30.2

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.

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.

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

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



--