From f3ab43accf65724c8b97550369fc21a2e652348d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Mon, 20 Mar 2023 17:36:41 +0400 Subject: [PATCH 1/7] win32: add qemu_close_socket_osfhandle() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Close the given file descriptor, but returns the underlying SOCKET. Signed-off-by: Marc-André Lureau Reviewed-by: Daniel P. Berrangé Message-Id: <20230320133643.1618437-2-marcandre.lureau@redhat.com> --- include/sysemu/os-win32.h | 15 ++++++-- util/oslib-win32.c | 75 +++++++++++++++++++++------------------ 2 files changed, 53 insertions(+), 37 deletions(-) diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h index e2849f88ab..15c296e0eb 100644 --- a/include/sysemu/os-win32.h +++ b/include/sysemu/os-win32.h @@ -171,10 +171,21 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject, bool qemu_socket_unselect(int sockfd, Error **errp); -/* We wrap all the sockets functions so that we can - * set errno based on WSAGetLastError() +/* We wrap all the sockets functions so that we can set errno based on + * WSAGetLastError(), and use file-descriptors instead of SOCKET. */ +/* + * qemu_close_socket_osfhandle: + * @fd: a file descriptor associated with a SOCKET + * + * Close only the C run-time file descriptor, leave the SOCKET opened. + * + * Returns zero on success. On error, -1 is returned, and errno is set to + * indicate the error. + */ +int qemu_close_socket_osfhandle(int fd); + #undef close #define close qemu_close_wrap int qemu_close_wrap(int fd); diff --git a/util/oslib-win32.c b/util/oslib-win32.c index 16f8a67f7e..a98638729a 100644 --- a/util/oslib-win32.c +++ b/util/oslib-win32.c @@ -479,40 +479,27 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr, return ret; } - #undef close -int qemu_close_wrap(int fd) +int qemu_close_socket_osfhandle(int fd) { - int ret; + SOCKET s = _get_osfhandle(fd); DWORD flags = 0; - SOCKET s = INVALID_SOCKET; - if (fd_is_socket(fd)) { - s = _get_osfhandle(fd); - - /* - * If we were to just call _close on the descriptor, it would close the - * HANDLE, but it wouldn't free any of the resources associated to the - * SOCKET, and we can't call _close after calling closesocket, because - * closesocket has already closed the HANDLE, and _close would attempt to - * close the HANDLE again, resulting in a double free. We can however - * protect the HANDLE from actually being closed long enough to close the - * file descriptor, then close the socket itself. - */ - if (!GetHandleInformation((HANDLE)s, &flags)) { - errno = EACCES; - return -1; - } - - if (!SetHandleInformation((HANDLE)s, HANDLE_FLAG_PROTECT_FROM_CLOSE, HANDLE_FLAG_PROTECT_FROM_CLOSE)) { - errno = EACCES; - return -1; - } + /* + * If we were to just call _close on the descriptor, it would close the + * HANDLE, but it wouldn't free any of the resources associated to the + * SOCKET, and we can't call _close after calling closesocket, because + * closesocket has already closed the HANDLE, and _close would attempt to + * close the HANDLE again, resulting in a double free. We can however + * protect the HANDLE from actually being closed long enough to close the + * file descriptor, then close the socket itself. + */ + if (!GetHandleInformation((HANDLE)s, &flags)) { + errno = EACCES; + return -1; } - ret = close(fd); - - if (s != INVALID_SOCKET && !SetHandleInformation((HANDLE)s, flags, flags)) { + if (!SetHandleInformation((HANDLE)s, HANDLE_FLAG_PROTECT_FROM_CLOSE, HANDLE_FLAG_PROTECT_FROM_CLOSE)) { errno = EACCES; return -1; } @@ -521,15 +508,33 @@ int qemu_close_wrap(int fd) * close() returns EBADF since we PROTECT_FROM_CLOSE the underlying handle, * but the FD is actually freed */ - if (ret < 0 && (s == INVALID_SOCKET || errno != EBADF)) { - return ret; + if (close(fd) < 0 && errno != EBADF) { + return -1; } - if (s != INVALID_SOCKET) { - ret = closesocket(s); - if (ret < 0) { - errno = socket_error(); - } + if (!SetHandleInformation((HANDLE)s, flags, flags)) { + errno = EACCES; + return -1; + } + + return 0; +} + +int qemu_close_wrap(int fd) +{ + SOCKET s = INVALID_SOCKET; + int ret = -1; + + if (!fd_is_socket(fd)) { + return close(fd); + } + + s = _get_osfhandle(fd); + qemu_close_socket_osfhandle(fd); + + ret = closesocket(s); + if (ret < 0) { + errno = socket_error(); } return ret; From e40283d9a13e9a26d58b089e243e46fe7724fe89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Mon, 20 Mar 2023 17:36:42 +0400 Subject: [PATCH 2/7] ui/spice: fix SOCKET handling regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spice uses SOCKET on win32, but QEMU now uses file-descriptors. Fixes "8.0.0rc0 Regression: spicy windows doesn't open": https://gitlab.com/qemu-project/qemu/-/issues/1549 Fixes: commit abe34282b ("win32: avoid mixing SOCKET and file descriptor space") Signed-off-by: Marc-André Lureau Reviewed-by: Daniel P. Berrangé Message-Id: <20230320133643.1618437-3-marcandre.lureau@redhat.com> --- ui/spice-core.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/ui/spice-core.c b/ui/spice-core.c index b05c830086..67cfd3ca9c 100644 --- a/ui/spice-core.c +++ b/ui/spice-core.c @@ -90,13 +90,23 @@ struct SpiceWatch { static void watch_read(void *opaque) { SpiceWatch *watch = opaque; - watch->func(watch->fd, SPICE_WATCH_EVENT_READ, watch->opaque); + int fd = watch->fd; + +#ifdef WIN32 + fd = _get_osfhandle(fd); +#endif + watch->func(fd, SPICE_WATCH_EVENT_READ, watch->opaque); } static void watch_write(void *opaque) { SpiceWatch *watch = opaque; - watch->func(watch->fd, SPICE_WATCH_EVENT_WRITE, watch->opaque); + int fd = watch->fd; + +#ifdef WIN32 + fd = _get_osfhandle(fd); +#endif + watch->func(fd, SPICE_WATCH_EVENT_WRITE, watch->opaque); } static void watch_update_mask(SpiceWatch *watch, int event_mask) @@ -117,6 +127,14 @@ static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void * { SpiceWatch *watch; +#ifdef WIN32 + fd = _open_osfhandle(fd, _O_BINARY); + if (fd < 0) { + error_setg_win32(&error_warn, WSAGetLastError(), "Couldn't associate a FD with the SOCKET"); + return NULL; + } +#endif + watch = g_malloc0(sizeof(*watch)); watch->fd = fd; watch->func = func; @@ -129,6 +147,10 @@ static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void * static void watch_remove(SpiceWatch *watch) { qemu_set_fd_handler(watch->fd, NULL, NULL, NULL); +#ifdef WIN32 + /* SOCKET is owned by spice */ + qemu_close_to_socket(watch->fd); +#endif g_free(watch); } @@ -908,6 +930,9 @@ static int qemu_spice_set_pw_expire(time_t expires) static int qemu_spice_display_add_client(int csock, int skipauth, int tls) { +#ifdef WIN32 + csock = qemu_close_socket_osfhandle(csock); +#endif if (tls) { return spice_server_add_ssl_client(spice_server, csock, skipauth); } else { From 74bc00c6b9065e34f04000a06c89cd04a814a599 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Mon, 20 Mar 2023 17:36:43 +0400 Subject: [PATCH 3/7] ui/dbus: fix passing SOCKET to GSocket API & leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit -display dbus is not currently available to win32 users, so it's not considered a regression. Note also the close() leak fix in case of error. Signed-off-by: Marc-André Lureau Reviewed-by: Daniel P. Berrangé Message-Id: <20230320133643.1618437-4-marcandre.lureau@redhat.com> --- ui/dbus.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ui/dbus.c b/ui/dbus.c index 0513de9918..b9e9698503 100644 --- a/ui/dbus.c +++ b/ui/dbus.c @@ -304,11 +304,20 @@ dbus_display_add_client(int csock, Error **errp) g_cancellable_cancel(dbus_display->add_client_cancellable); } +#ifdef WIN32 + socket = g_socket_new_from_fd(_get_osfhandle(csock), &err); +#else socket = g_socket_new_from_fd(csock, &err); +#endif if (!socket) { error_setg(errp, "Failed to setup D-Bus socket: %s", err->message); + close(csock); return false; } +#ifdef WIN32 + /* socket owns the SOCKET handle now, so release our osf handle */ + qemu_close_socket_osfhandle(csock); +#endif conn = g_socket_connection_factory_create_connection(socket); From 281a77df28bdea2b4c475a26d4c2d92b2d931af9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Mon, 20 Mar 2023 17:26:24 +0400 Subject: [PATCH 4/7] ui/gtk: fix cursor moved to left corner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Do not attempt to move the pointer if the widget is not yet realized. The mouse cursor is placed to the corner of the screen, on X11 at least, as x_root and y_root are then miscalculated. (this is not reproducible on Wayland, because Gtk doesn't implement device warping there) This also fixes the following warning at start: qemu: Gdk: gdk_window_get_root_coords: assertion 'GDK_IS_WINDOW (window)' failed Fixes: 6effaa16ac98 ("ui: set cursor position upon listener registration") Reported-by: Bernhard Beschow Signed-off-by: Marc-André Lureau Reviewed-by: Daniel P. Berrangé Tested-by: Bernhard Beschow Message-Id: <20230320132624.1612464-1-marcandre.lureau@redhat.com> --- ui/gtk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ui/gtk.c b/ui/gtk.c index fd82e9b1ca..e9564f2baa 100644 --- a/ui/gtk.c +++ b/ui/gtk.c @@ -450,7 +450,8 @@ static void gd_mouse_set(DisplayChangeListener *dcl, GdkDisplay *dpy; gint x_root, y_root; - if (qemu_input_is_absolute()) { + if (!gtk_widget_get_realized(vc->gfx.drawing_area) || + qemu_input_is_absolute()) { return; } From 3c293a46627063eb4b12d808c7ec43b1cff8e463 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Sun, 19 Mar 2023 15:10:17 +0400 Subject: [PATCH 5/7] ui: return the default console cursor when con == NULL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VNC code relies on con==NULL to mean the default console. Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1548 Fixes: commit 385ac97f8 ("ui: keep current cursor with QemuConsole") Signed-off-by: Marc-André Lureau Reported-by: Helge Konetzka Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20230319111017.1319880-1-marcandre.lureau@redhat.com> --- ui/console.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ui/console.c b/ui/console.c index f3783021e5..6e8a3cdc62 100644 --- a/ui/console.c +++ b/ui/console.c @@ -2303,6 +2303,9 @@ QemuConsole *qemu_console_lookup_unused(void) QEMUCursor *qemu_console_get_cursor(QemuConsole *con) { + if (con == NULL) { + con = active_console; + } return con->cursor; } From 9b6611f40b08e255e28ebc615c64df0252dacbb8 Mon Sep 17 00:00:00 2001 From: Erico Nunes Date: Wed, 1 Mar 2023 15:12:05 +0100 Subject: [PATCH 6/7] ui/sdl2: remove workaround forcing x11 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This workaround was put in place in the original implementation almost 10 years ago, considering a very old SDL2 version. Currently it prevents users to run in a wayland-only environment without manually forcing the backend. The SDL2 wayland backend has been supported by distributions for a very long time (e.g. in Fedora, first available 8 years ago), and is now considered stable and becoming the default for new SDL2 releases. Instead of requiring the x11 backend to exist by default, let new qemu releases run with the default chosen by the installed SDL2 version. Signed-off-by: Erico Nunes Reviewed-by: Daniel P. Berrangé Reviewed-by: Marc-André Lureau Message-Id: <20230301141205.514338-1-ernunes@redhat.com> --- ui/sdl2.c | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/ui/sdl2.c b/ui/sdl2.c index 35c58c1104..b12dec4caf 100644 --- a/ui/sdl2.c +++ b/ui/sdl2.c @@ -843,22 +843,6 @@ static void sdl2_display_init(DisplayState *ds, DisplayOptions *o) assert(o->type == DISPLAY_TYPE_SDL); -#ifdef __linux__ - /* on Linux, SDL may use fbcon|directfb|svgalib when run without - * accessible $DISPLAY to open X11 window. This is often the case - * when qemu is run using sudo. But in this case, and when actually - * run in X11 environment, SDL fights with X11 for the video card, - * making current display unavailable, often until reboot. - * So make x11 the default SDL video driver if this variable is unset. - * This is a bit hackish but saves us from bigger problem. - * Maybe it's a good idea to fix this in SDL instead. - */ - if (!g_setenv("SDL_VIDEODRIVER", "x11", 0)) { - fprintf(stderr, "Could not set SDL_VIDEODRIVER environment variable\n"); - exit(1); - } -#endif - if (SDL_GetHintBoolean("QEMU_ENABLE_SDL_LOGGING", SDL_FALSE)) { SDL_LogSetAllPriority(SDL_LOG_PRIORITY_VERBOSE); } From 49152ac47003ca21fc6f2a5c3e517f79649e1541 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Mon, 20 Feb 2023 11:22:51 +0400 Subject: [PATCH 7/7] ui: fix crash on serial reset, during init MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For ex, when resetting the xlnx-zcu102 machine: (lldb) bt * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x50) * frame #0: 0x10020a740 gd_vc_send_chars(vc=0x000000000) at gtk.c:1759:41 [opt] frame #1: 0x100636264 qemu_chr_fe_accept_input(be=) at char-fe.c:159:9 [opt] frame #2: 0x1000608e0 cadence_uart_reset_hold [inlined] uart_rx_reset(s=0x10810a960) at cadence_uart.c:158:5 [opt] frame #3: 0x1000608d4 cadence_uart_reset_hold(obj=0x10810a960) at cadence_uart.c:530:5 [opt] frame #4: 0x100580ab4 resettable_phase_hold(obj=0x10810a960, opaque=0x000000000, type=) at resettable.c:0 [opt] frame #5: 0x10057d1b0 bus_reset_child_foreach(obj=, cb=(resettable_phase_hold at resettable.c:162), opaque=0x000000000, type=RESET_TYPE_COLD) at bus.c:97:13 [opt] frame #6: 0x1005809f8 resettable_phase_hold [inlined] resettable_child_foreach(rc=0x000060000332d2c0, obj=0x0000600002c1c180, cb=, opaque=0x000000000, type=RESET_TYPE_COLD) at resettable.c:96:9 [opt] frame #7: 0x1005809d8 resettable_phase_hold(obj=0x0000600002c1c180, opaque=0x000000000, type=RESET_TYPE_COLD) at resettable.c:173:5 [opt] frame #8: 0x1005803a0 resettable_assert_reset(obj=0x0000600002c1c180, type=) at resettable.c:60:5 [opt] frame #9: 0x10058027c resettable_reset(obj=0x0000600002c1c180, type=RESET_TYPE_COLD) at resettable.c:45:5 [opt] While the chardev is created early, the VirtualConsole is associated after, during qemu_init_displays(). Signed-off-by: Marc-André Lureau Tested-by: Philippe Mathieu-Daudé Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20230220072251.3385878-1-marcandre.lureau@redhat.com> --- ui/gtk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ui/gtk.c b/ui/gtk.c index e9564f2baa..f16e0f8dee 100644 --- a/ui/gtk.c +++ b/ui/gtk.c @@ -1784,7 +1784,9 @@ static void gd_vc_chr_accept_input(Chardev *chr) VCChardev *vcd = VC_CHARDEV(chr); VirtualConsole *vc = vcd->console; - gd_vc_send_chars(vc); + if (vc) { + gd_vc_send_chars(vc); + } } static void gd_vc_chr_set_echo(Chardev *chr, bool echo)