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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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();
>>
>