[16/16] tests: add basic -display dbus Map.Unix test

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Only check we eventually get a shared memory scanout.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/qtest/dbus-display-test.c | 64 ++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 5 deletions(-)

On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Only check we eventually get a shared memory scanout.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   tests/qtest/dbus-display-test.c | 64 ++++++++++++++++++++++++++++++---
>   1 file changed, 59 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/qtest/dbus-display-test.c b/tests/qtest/dbus-display-test.c
> index 0390bdcb41..ac92cb00d4 100644
> --- a/tests/qtest/dbus-display-test.c
> +++ b/tests/qtest/dbus-display-test.c
> @@ -2,9 +2,12 @@
>   #include "qemu/sockets.h"
>   #include "qemu/dbus.h"
>   #include "qemu/sockets.h"
> +#include "glib.h"
> +#include "glibconfig.h"
>   #include <gio/gio.h>
>   #include <gio/gunixfdlist.h>
>   #include "libqtest.h"
> +#include <sys/mman.h>
>   #include "ui/dbus-display1.h"
>   
>   static GDBusConnection*
> @@ -82,6 +85,7 @@ typedef struct TestDBusConsoleRegister {
>       GThread *thread;
>       GDBusConnection *listener_conn;
>       GDBusObjectManagerServer *server;
> +    bool with_map;
>   } TestDBusConsoleRegister;
>   
>   static gboolean listener_handle_scanout(
> @@ -94,13 +98,48 @@ static gboolean listener_handle_scanout(
>       GVariant *arg_data,
>       TestDBusConsoleRegister *test)
>   {
> +    if (!test->with_map) {
> +        g_main_loop_quit(test->loop);
> +    }
> +
> +    return DBUS_METHOD_INVOCATION_HANDLED;
> +}
> +
> +static gboolean listener_handle_scanout_map(
> +    QemuDBusDisplay1ListenerUnixMap *object,
> +    GDBusMethodInvocation *invocation,
> +    GUnixFDList *fd_list,
> +    GVariant *arg_handle,
> +    guint arg_offset,
> +    guint arg_width,
> +    guint arg_height,
> +    guint arg_stride,
> +    guint arg_pixman_format,
> +    TestDBusConsoleRegister *test)
> +{
> +    int fd = -1;
> +    gint32 handle = g_variant_get_handle(arg_handle);
> +    g_autoptr(GError) error = NULL;
> +    void *addr = NULL;
> +    size_t len = arg_height * arg_stride;
> +
> +    g_assert_cmpuint(g_unix_fd_list_get_length(fd_list), ==, 1);
> +    fd = g_unix_fd_list_get(fd_list, handle, &error);
> +    g_assert_no_error(error);
> +
> +    addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, arg_offset);
> +    g_assert_no_errno(GPOINTER_TO_INT(addr));

Strictly speaking, this construct is not safe. When void * is 64-bit and 
int is 32-bit, this assetion will fail if the lower 32 bits of addr are 
in [0x80000000, 0xffffffff] though addr may still be a valid address. 
This is because GPOINTER_TO_INT() results in a negative value for such 
an address, and g_assert_no_errno() asserts that the given value is 
non-negative.

Using g_mapped_file_new_from_fd() and will simplify this function as whole.

> +    g_assert_nonnull(addr);
> +    g_assert_no_errno(munmap(addr, len));
> +
>       g_main_loop_quit(test->loop);
>   
> +    close(fd);
>       return DBUS_METHOD_INVOCATION_HANDLED;
>   }
>   
>   static void
> -test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
> +test_dbus_console_setup_listener(TestDBusConsoleRegister *test, bool with_map)
>   {
>       g_autoptr(GDBusObjectSkeleton) listener = NULL;
>       g_autoptr(QemuDBusDisplay1ListenerSkeleton) iface = NULL;
> @@ -114,6 +153,20 @@ test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
>                        NULL);
>       g_dbus_object_skeleton_add_interface(listener,
>                                            G_DBUS_INTERFACE_SKELETON(iface));
> +    if (with_map) {
> +        g_autoptr(QemuDBusDisplay1ListenerUnixMapSkeleton) iface_map =
> +            QEMU_DBUS_DISPLAY1_LISTENER_UNIX_MAP_SKELETON(
> +                qemu_dbus_display1_listener_unix_map_skeleton_new());
> +
> +        g_object_connect(iface_map,
> +                         "signal::handle-scanout-map", listener_handle_scanout_map, test,
> +                         NULL);
> +        g_dbus_object_skeleton_add_interface(listener,
> +                                             G_DBUS_INTERFACE_SKELETON(iface_map));
> +        g_object_set(iface, "interfaces",
> +            (const gchar *[]) { "org.qemu.Display1.Listener.Unix.Map", NULL },
> +            NULL);
> +    }
>       g_dbus_object_manager_server_export(test->server, listener);
>       g_dbus_object_manager_server_set_connection(test->server,
>                                                   test->listener_conn);
> @@ -145,7 +198,7 @@ test_dbus_console_registered(GObject *source_object,
>       g_assert_no_error(err);
>   
>       test->listener_conn = g_thread_join(test->thread);
> -    test_dbus_console_setup_listener(test);
> +    test_dbus_console_setup_listener(test, test->with_map);
>   }
>   
>   static gpointer
> @@ -155,7 +208,7 @@ test_dbus_p2p_server_setup_thread(gpointer data)
>   }
>   
>   static void
> -test_dbus_display_console(void)
> +test_dbus_display_console(const void* data)
>   {
>       g_autoptr(GError) err = NULL;
>       g_autoptr(GDBusConnection) conn = NULL;
> @@ -163,7 +216,7 @@ test_dbus_display_console(void)
>       g_autoptr(GMainLoop) loop = NULL;
>       QTestState *qts = NULL;
>       int pair[2];
> -    TestDBusConsoleRegister test = { 0, };
> +    TestDBusConsoleRegister test = { 0, .with_map = GPOINTER_TO_INT(data) };
>   #ifdef WIN32
>       WSAPROTOCOL_INFOW info;
>       g_autoptr(GVariant) listener = NULL;
> @@ -299,7 +352,8 @@ main(int argc, char **argv)
>       g_test_init(&argc, &argv, NULL);
>   
>       qtest_add_func("/dbus-display/vm", test_dbus_display_vm);
> -    qtest_add_func("/dbus-display/console", test_dbus_display_console);
> +    qtest_add_data_func("/dbus-display/console", GINT_TO_POINTER(false), test_dbus_display_console);
> +    qtest_add_data_func("/dbus-display/console/map", GINT_TO_POINTER(true), test_dbus_display_console);
>       qtest_add_func("/dbus-display/keyboard", test_dbus_display_keyboard);
>   
>       return g_test_run();

Hi

On Sat, Oct 5, 2024 at 12:32 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Only check we eventually get a shared memory scanout.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   tests/qtest/dbus-display-test.c | 64 ++++++++++++++++++++++++++++++---
> >   1 file changed, 59 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/qtest/dbus-display-test.c b/tests/qtest/dbus-display-test.c
> > index 0390bdcb41..ac92cb00d4 100644
> > --- a/tests/qtest/dbus-display-test.c
> > +++ b/tests/qtest/dbus-display-test.c
> > @@ -2,9 +2,12 @@
> >   #include "qemu/sockets.h"
> >   #include "qemu/dbus.h"
> >   #include "qemu/sockets.h"
> > +#include "glib.h"
> > +#include "glibconfig.h"
> >   #include <gio/gio.h>
> >   #include <gio/gunixfdlist.h>
> >   #include "libqtest.h"
> > +#include <sys/mman.h>
> >   #include "ui/dbus-display1.h"
> >
> >   static GDBusConnection*
> > @@ -82,6 +85,7 @@ typedef struct TestDBusConsoleRegister {
> >       GThread *thread;
> >       GDBusConnection *listener_conn;
> >       GDBusObjectManagerServer *server;
> > +    bool with_map;
> >   } TestDBusConsoleRegister;
> >
> >   static gboolean listener_handle_scanout(
> > @@ -94,13 +98,48 @@ static gboolean listener_handle_scanout(
> >       GVariant *arg_data,
> >       TestDBusConsoleRegister *test)
> >   {
> > +    if (!test->with_map) {
> > +        g_main_loop_quit(test->loop);
> > +    }
> > +
> > +    return DBUS_METHOD_INVOCATION_HANDLED;
> > +}
> > +
> > +static gboolean listener_handle_scanout_map(
> > +    QemuDBusDisplay1ListenerUnixMap *object,
> > +    GDBusMethodInvocation *invocation,
> > +    GUnixFDList *fd_list,
> > +    GVariant *arg_handle,
> > +    guint arg_offset,
> > +    guint arg_width,
> > +    guint arg_height,
> > +    guint arg_stride,
> > +    guint arg_pixman_format,
> > +    TestDBusConsoleRegister *test)
> > +{
> > +    int fd = -1;
> > +    gint32 handle = g_variant_get_handle(arg_handle);
> > +    g_autoptr(GError) error = NULL;
> > +    void *addr = NULL;
> > +    size_t len = arg_height * arg_stride;
> > +
> > +    g_assert_cmpuint(g_unix_fd_list_get_length(fd_list), ==, 1);
> > +    fd = g_unix_fd_list_get(fd_list, handle, &error);
> > +    g_assert_no_error(error);
> > +
> > +    addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, arg_offset);
> > +    g_assert_no_errno(GPOINTER_TO_INT(addr));
>
> Strictly speaking, this construct is not safe. When void * is 64-bit and
> int is 32-bit, this assetion will fail if the lower 32 bits of addr are
> in [0x80000000, 0xffffffff] though addr may still be a valid address.
> This is because GPOINTER_TO_INT() results in a negative value for such
> an address, and g_assert_no_errno() asserts that the given value is
> non-negative.
>
> Using g_mapped_file_new_from_fd() and will simplify this function as whole.

I thought about that, but g_mapped_file_new_from_fd() doesn't take a
len, and it fstat() the fd. This is ok for memfd apparently, and
appears to work, but it isn't really compliant with the dbus
interface.

>
> > +    g_assert_nonnull(addr);
> > +    g_assert_no_errno(munmap(addr, len));
> > +
> >       g_main_loop_quit(test->loop);
> >
> > +    close(fd);
> >       return DBUS_METHOD_INVOCATION_HANDLED;
> >   }
> >
> >   static void
> > -test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
> > +test_dbus_console_setup_listener(TestDBusConsoleRegister *test, bool with_map)
> >   {
> >       g_autoptr(GDBusObjectSkeleton) listener = NULL;
> >       g_autoptr(QemuDBusDisplay1ListenerSkeleton) iface = NULL;
> > @@ -114,6 +153,20 @@ test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
> >                        NULL);
> >       g_dbus_object_skeleton_add_interface(listener,
> >                                            G_DBUS_INTERFACE_SKELETON(iface));
> > +    if (with_map) {
> > +        g_autoptr(QemuDBusDisplay1ListenerUnixMapSkeleton) iface_map =
> > +            QEMU_DBUS_DISPLAY1_LISTENER_UNIX_MAP_SKELETON(
> > +                qemu_dbus_display1_listener_unix_map_skeleton_new());
> > +
> > +        g_object_connect(iface_map,
> > +                         "signal::handle-scanout-map", listener_handle_scanout_map, test,
> > +                         NULL);
> > +        g_dbus_object_skeleton_add_interface(listener,
> > +                                             G_DBUS_INTERFACE_SKELETON(iface_map));
> > +        g_object_set(iface, "interfaces",
> > +            (const gchar *[]) { "org.qemu.Display1.Listener.Unix.Map", NULL },
> > +            NULL);
> > +    }
> >       g_dbus_object_manager_server_export(test->server, listener);
> >       g_dbus_object_manager_server_set_connection(test->server,
> >                                                   test->listener_conn);
> > @@ -145,7 +198,7 @@ test_dbus_console_registered(GObject *source_object,
> >       g_assert_no_error(err);
> >
> >       test->listener_conn = g_thread_join(test->thread);
> > -    test_dbus_console_setup_listener(test);
> > +    test_dbus_console_setup_listener(test, test->with_map);
> >   }
> >
> >   static gpointer
> > @@ -155,7 +208,7 @@ test_dbus_p2p_server_setup_thread(gpointer data)
> >   }
> >
> >   static void
> > -test_dbus_display_console(void)
> > +test_dbus_display_console(const void* data)
> >   {
> >       g_autoptr(GError) err = NULL;
> >       g_autoptr(GDBusConnection) conn = NULL;
> > @@ -163,7 +216,7 @@ test_dbus_display_console(void)
> >       g_autoptr(GMainLoop) loop = NULL;
> >       QTestState *qts = NULL;
> >       int pair[2];
> > -    TestDBusConsoleRegister test = { 0, };
> > +    TestDBusConsoleRegister test = { 0, .with_map = GPOINTER_TO_INT(data) };
> >   #ifdef WIN32
> >       WSAPROTOCOL_INFOW info;
> >       g_autoptr(GVariant) listener = NULL;
> > @@ -299,7 +352,8 @@ main(int argc, char **argv)
> >       g_test_init(&argc, &argv, NULL);
> >
> >       qtest_add_func("/dbus-display/vm", test_dbus_display_vm);
> > -    qtest_add_func("/dbus-display/console", test_dbus_display_console);
> > +    qtest_add_data_func("/dbus-display/console", GINT_TO_POINTER(false), test_dbus_display_console);
> > +    qtest_add_data_func("/dbus-display/console/map", GINT_TO_POINTER(true), test_dbus_display_console);
> >       qtest_add_func("/dbus-display/keyboard", test_dbus_display_keyboard);
> >
> >       return g_test_run();
>

On 2024/10/07 21:42, Marc-André Lureau wrote:
> Hi
> 
> On Sat, Oct 5, 2024 at 12:32 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> Only check we eventually get a shared memory scanout.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>    tests/qtest/dbus-display-test.c | 64 ++++++++++++++++++++++++++++++---
>>>    1 file changed, 59 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tests/qtest/dbus-display-test.c b/tests/qtest/dbus-display-test.c
>>> index 0390bdcb41..ac92cb00d4 100644
>>> --- a/tests/qtest/dbus-display-test.c
>>> +++ b/tests/qtest/dbus-display-test.c
>>> @@ -2,9 +2,12 @@
>>>    #include "qemu/sockets.h"
>>>    #include "qemu/dbus.h"
>>>    #include "qemu/sockets.h"
>>> +#include "glib.h"
>>> +#include "glibconfig.h"
>>>    #include <gio/gio.h>
>>>    #include <gio/gunixfdlist.h>
>>>    #include "libqtest.h"
>>> +#include <sys/mman.h>
>>>    #include "ui/dbus-display1.h"
>>>
>>>    static GDBusConnection*
>>> @@ -82,6 +85,7 @@ typedef struct TestDBusConsoleRegister {
>>>        GThread *thread;
>>>        GDBusConnection *listener_conn;
>>>        GDBusObjectManagerServer *server;
>>> +    bool with_map;
>>>    } TestDBusConsoleRegister;
>>>
>>>    static gboolean listener_handle_scanout(
>>> @@ -94,13 +98,48 @@ static gboolean listener_handle_scanout(
>>>        GVariant *arg_data,
>>>        TestDBusConsoleRegister *test)
>>>    {
>>> +    if (!test->with_map) {
>>> +        g_main_loop_quit(test->loop);
>>> +    }
>>> +
>>> +    return DBUS_METHOD_INVOCATION_HANDLED;
>>> +}
>>> +
>>> +static gboolean listener_handle_scanout_map(
>>> +    QemuDBusDisplay1ListenerUnixMap *object,
>>> +    GDBusMethodInvocation *invocation,
>>> +    GUnixFDList *fd_list,
>>> +    GVariant *arg_handle,
>>> +    guint arg_offset,
>>> +    guint arg_width,
>>> +    guint arg_height,
>>> +    guint arg_stride,
>>> +    guint arg_pixman_format,
>>> +    TestDBusConsoleRegister *test)
>>> +{
>>> +    int fd = -1;
>>> +    gint32 handle = g_variant_get_handle(arg_handle);
>>> +    g_autoptr(GError) error = NULL;
>>> +    void *addr = NULL;
>>> +    size_t len = arg_height * arg_stride;
>>> +
>>> +    g_assert_cmpuint(g_unix_fd_list_get_length(fd_list), ==, 1);
>>> +    fd = g_unix_fd_list_get(fd_list, handle, &error);
>>> +    g_assert_no_error(error);
>>> +
>>> +    addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, arg_offset);
>>> +    g_assert_no_errno(GPOINTER_TO_INT(addr));
>>
>> Strictly speaking, this construct is not safe. When void * is 64-bit and
>> int is 32-bit, this assetion will fail if the lower 32 bits of addr are
>> in [0x80000000, 0xffffffff] though addr may still be a valid address.
>> This is because GPOINTER_TO_INT() results in a negative value for such
>> an address, and g_assert_no_errno() asserts that the given value is
>> non-negative.
>>
>> Using g_mapped_file_new_from_fd() and will simplify this function as whole.
> 
> I thought about that, but g_mapped_file_new_from_fd() doesn't take a
> len, and it fstat() the fd. This is ok for memfd apparently, and
> appears to work, but it isn't really compliant with the dbus
> interface.

That's unfortunate. It seems we need to do:
g_assert_no_errno(addr == MAP_FAILED ? -1 : 0)

g_assert_nonnull() following this should be unnecessary by the way. It 
shouldn't happen unless the underlying platform is broken, which is 
irrelevant from the correctness of our code.

> 
>>
>>> +    g_assert_nonnull(addr);
>>> +    g_assert_no_errno(munmap(addr, len));
>>> +
>>>        g_main_loop_quit(test->loop);
>>>
>>> +    close(fd);
>>>        return DBUS_METHOD_INVOCATION_HANDLED;
>>>    }
>>>
>>>    static void
>>> -test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
>>> +test_dbus_console_setup_listener(TestDBusConsoleRegister *test, bool with_map)
>>>    {
>>>        g_autoptr(GDBusObjectSkeleton) listener = NULL;
>>>        g_autoptr(QemuDBusDisplay1ListenerSkeleton) iface = NULL;
>>> @@ -114,6 +153,20 @@ test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
>>>                         NULL);
>>>        g_dbus_object_skeleton_add_interface(listener,
>>>                                             G_DBUS_INTERFACE_SKELETON(iface));
>>> +    if (with_map) {
>>> +        g_autoptr(QemuDBusDisplay1ListenerUnixMapSkeleton) iface_map =
>>> +            QEMU_DBUS_DISPLAY1_LISTENER_UNIX_MAP_SKELETON(
>>> +                qemu_dbus_display1_listener_unix_map_skeleton_new());
>>> +
>>> +        g_object_connect(iface_map,
>>> +                         "signal::handle-scanout-map", listener_handle_scanout_map, test,
>>> +                         NULL);
>>> +        g_dbus_object_skeleton_add_interface(listener,
>>> +                                             G_DBUS_INTERFACE_SKELETON(iface_map));
>>> +        g_object_set(iface, "interfaces",
>>> +            (const gchar *[]) { "org.qemu.Display1.Listener.Unix.Map", NULL },
>>> +            NULL);
>>> +    }
>>>        g_dbus_object_manager_server_export(test->server, listener);
>>>        g_dbus_object_manager_server_set_connection(test->server,
>>>                                                    test->listener_conn);
>>> @@ -145,7 +198,7 @@ test_dbus_console_registered(GObject *source_object,
>>>        g_assert_no_error(err);
>>>
>>>        test->listener_conn = g_thread_join(test->thread);
>>> -    test_dbus_console_setup_listener(test);
>>> +    test_dbus_console_setup_listener(test, test->with_map);
>>>    }
>>>
>>>    static gpointer
>>> @@ -155,7 +208,7 @@ test_dbus_p2p_server_setup_thread(gpointer data)
>>>    }
>>>
>>>    static void
>>> -test_dbus_display_console(void)
>>> +test_dbus_display_console(const void* data)
>>>    {
>>>        g_autoptr(GError) err = NULL;
>>>        g_autoptr(GDBusConnection) conn = NULL;
>>> @@ -163,7 +216,7 @@ test_dbus_display_console(void)
>>>        g_autoptr(GMainLoop) loop = NULL;
>>>        QTestState *qts = NULL;
>>>        int pair[2];
>>> -    TestDBusConsoleRegister test = { 0, };
>>> +    TestDBusConsoleRegister test = { 0, .with_map = GPOINTER_TO_INT(data) };
>>>    #ifdef WIN32
>>>        WSAPROTOCOL_INFOW info;
>>>        g_autoptr(GVariant) listener = NULL;
>>> @@ -299,7 +352,8 @@ main(int argc, char **argv)
>>>        g_test_init(&argc, &argv, NULL);
>>>
>>>        qtest_add_func("/dbus-display/vm", test_dbus_display_vm);
>>> -    qtest_add_func("/dbus-display/console", test_dbus_display_console);
>>> +    qtest_add_data_func("/dbus-display/console", GINT_TO_POINTER(false), test_dbus_display_console);
>>> +    qtest_add_data_func("/dbus-display/console/map", GINT_TO_POINTER(true), test_dbus_display_console);
>>>        qtest_add_func("/dbus-display/keyboard", test_dbus_display_keyboard);
>>>
>>>        return g_test_run();
>>
>