Hi
On Sat, Oct 5, 2024 at 12:32 PM Akihiko Odaki <akihiko.od...@daynix.com> wrote:
>
> On 2024/10/03 20:22, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > Only check we eventually get a shared memory scanout.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@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();
>
