Re: [PATCH 5/6] tests/qtest: Reorganize common code in ivshmem-test

[Thomas Huth]

On 22/02/2024 23.22, Gustavo Romero wrote:
> This commit reorganizes the ivshmem-test qtest by moving common structs,
> functions, and code that can be utilized by other ivshmem qtests into
> two new files: ivshmem-utils.h and ivshmem-utils.c.
>
> Enum Reg, struct ServerThread, and mktempshm() have been relocated to
> these new files. Two new functions have been introduced to handle the
> ivshmem server start/stop: test_ivshmem_server_{start,stop}.
>
> To accommodate the new way for starting/stopping the ivshmem server,
> struct ServerThread now includes two new members: 'server', previously
> present but not a member of any struct; and 'status', a new member of a
> new type, ServerStartStatus, used to track and handle service
> termination properly.
>
> Additionally, a new function, mktempsocket(), has been added to help
> create a unix socket filename, similar to what mktempshm() does for the
> creation of a shm file.
>
> Finally, the ivshmem-test qtest has been adapted to use the new ivhsmem
> utils. Adjustments in that sense have also been made to meson.build;
> also 'rt' have been removed as a lib dependency for ivhsmem-test.c.
>
> Two lines unrelated to these changes have had their line indentation
> also fixed in meson.build.
>
> Message-ID: <20231127052024.435743-3-gustavo.romero@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
...
> diff --git a/tests/qtest/ivshmem-utils.c b/tests/qtest/ivshmem-utils.c
> new file mode 100644
> index 0000000000..c2fc3463dd
> --- /dev/null
> +++ b/tests/qtest/ivshmem-utils.c
> @@ -0,0 +1,156 @@
> +/*
> + * Common utilities for testing ivshmem devices
> + *
> + * SPDX-FileCopyrightText: 2012 SUSE LINUX Products GmbH
> + * SPDX-FileCopyrightText: 2021 Red Hat, Inc.
> + * SPDX-FileCopyrightText: 2023 Linaro Ltd.
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + */
> +
> +#include "ivshmem-utils.h"
> +
> +gchar *mktempshm(int size, int *fd)
> +{
> +    while (true) {
> +        /* Relative path to the shm filesystem, e.g. '/dev/shm'. */
> +        gchar *shm_rel_path;
> +
> +        shm_rel_path = g_strdup_printf("/ivshmem_qtest-%u-%u", getpid(),
> +                                       g_test_rand_int());
> +        *fd = shm_open(shm_rel_path, O_CREAT | O_RDWR | O_EXCL,
> +                       S_IRWXU | S_IRWXG | S_IRWXO);
> +        if (*fd > 0) {
> +            g_assert(ftruncate(*fd, size) == 0);
> +            return shm_rel_path;
> +        }
> +
> +        g_free(shm_rel_path);
> +
> +        if (errno != EEXIST) {
> +            perror("shm_open");
> +            return NULL;
> +        }
> +    }
> +}
> +
> +gchar *mktempsocket(void)
> +{
> +    gchar *server_socket_path;
> +
> +    server_socket_path = g_strdup_printf("%s/ivshmem_socket_qtest-%u-%u",
> +                                         g_get_tmp_dir(), getpid(),
> +                                         g_test_rand_int());
> +    return server_socket_path;
> +}

You could simplify that to:

    return g_strdup_printf("%s/ivshmem_socket_qtest-%u-%u",
                           g_get_tmp_dir(), getpid(),
                           g_test_rand_int());

Anyway:
Acked-by: Thomas Huth <thuth@redhat.com>