[PULL 0/7] ui/ fixes for 8.0

[PULL 0/7] ui/ fixes for 8.0

[marcandre.lureau]

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

The following changes since commit aa9e7fa4689d1becb2faf67f65aafcbcf664f1ce:

  Merge tag 'edk2-stable202302-20230320-pull-request' of https://gitlab.com/kraxel/qemu into staging (2023-03-20 13:43:35 +0000)

are available in the Git repository at:

  https://gitlab.com/marcandre.lureau/qemu.git tags/ui-pull-request

for you to fetch changes up to 49152ac47003ca21fc6f2a5c3e517f79649e1541:

  ui: fix crash on serial reset, during init (2023-03-21 11:46:22 +0400)

----------------------------------------------------------------
ui/ fixes for 8.0

----------------------------------------------------------------

Erico Nunes (1):
  ui/sdl2: remove workaround forcing x11

Marc-André Lureau (6):
  win32: add qemu_close_socket_osfhandle()
  ui/spice: fix SOCKET handling regression
  ui/dbus: fix passing SOCKET to GSocket API & leak
  ui/gtk: fix cursor moved to left corner
  ui: return the default console cursor when con == NULL
  ui: fix crash on serial reset, during init

 include/sysemu/os-win32.h | 15 ++++++--
 ui/console.c              |  3 ++
 ui/dbus.c                 |  9 +++++
 ui/gtk.c                  |  7 ++--
 ui/sdl2.c                 | 16 ---------
 ui/spice-core.c           | 29 +++++++++++++--
 util/oslib-win32.c        | 75 +++++++++++++++++++++------------------
 7 files changed, 97 insertions(+), 57 deletions(-)

-- 
2.39.2

[PULL 1/7] win32: add qemu_close_socket_osfhandle()

[marcandre.lureau]

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

Close the given file descriptor, but returns the underlying SOCKET.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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;
-- 
2.39.2

[PULL 2/7] ui/spice: fix SOCKET handling regression

[marcandre.lureau]

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

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 <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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 {
-- 
2.39.2

[PULL 3/7] ui/dbus: fix passing SOCKET to GSocket API & leak

[marcandre.lureau]

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

-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 <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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);
 
-- 
2.39.2

[PULL 4/7] ui/gtk: fix cursor moved to left corner

[marcandre.lureau]

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

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 <shentey@gmail.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Tested-by: Bernhard Beschow <shentey@gmail.com>
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;
     }
 
-- 
2.39.2

[PULL 5/7] ui: return the default console cursor when con == NULL

[marcandre.lureau]

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

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 <marcandre.lureau@redhat.com>
Reported-by: Helge Konetzka <hk@zapateado.de>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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;
 }
 
-- 
2.39.2

[PULL 6/7] ui/sdl2: remove workaround forcing x11

[marcandre.lureau]

From: Erico Nunes <ernunes@redhat.com>

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 <ernunes@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
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);
     }
-- 
2.39.2

[PULL 7/7] ui: fix crash on serial reset, during init

[marcandre.lureau]

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

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=<unavailable>) 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=<unavailable>) at resettable.c:0 [opt]
     frame #5: 0x10057d1b0 bus_reset_child_foreach(obj=<unavailable>,
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=<unavailable>, 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=<unavailable>) 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 <marcandre.lureau@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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)
-- 
2.39.2

Re: [PULL 0/7] ui/ fixes for 8.0

[Erico Nunes]

On 21/03/2023 10:03, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The following changes since commit aa9e7fa4689d1becb2faf67f65aafcbcf664f1ce:
> 
>   Merge tag 'edk2-stable202302-20230320-pull-request' of https://gitlab.com/kraxel/qemu into staging (2023-03-20 13:43:35 +0000)
> 
> are available in the Git repository at:
> 
>   https://gitlab.com/marcandre.lureau/qemu.git tags/ui-pull-request
> 
> for you to fetch changes up to 49152ac47003ca21fc6f2a5c3e517f79649e1541:
> 
>   ui: fix crash on serial reset, during init (2023-03-21 11:46:22 +0400)
> 
> ----------------------------------------------------------------
> ui/ fixes for 8.0
> 
> ----------------------------------------------------------------
> 
> Erico Nunes (1):
>   ui/sdl2: remove workaround forcing x11
> 
> Marc-André Lureau (6):
>   win32: add qemu_close_socket_osfhandle()
>   ui/spice: fix SOCKET handling regression
>   ui/dbus: fix passing SOCKET to GSocket API & leak
>   ui/gtk: fix cursor moved to left corner
>   ui: return the default console cursor when con == NULL
>   ui: fix crash on serial reset, during init


May I also suggest this one as a fix for 8.0:
https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg05667.html

It was already reviewed about a month ago.

Thanks

Erico

Re: [PULL 0/7] ui/ fixes for 8.0

[Peter Maydell]

On Tue, 21 Mar 2023 at 09:04, <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The following changes since commit aa9e7fa4689d1becb2faf67f65aafcbcf664f1ce:
>
>   Merge tag 'edk2-stable202302-20230320-pull-request' of https://gitlab.com/kraxel/qemu into staging (2023-03-20 13:43:35 +0000)
>
> are available in the Git repository at:
>
>   https://gitlab.com/marcandre.lureau/qemu.git tags/ui-pull-request
>
> for you to fetch changes up to 49152ac47003ca21fc6f2a5c3e517f79649e1541:
>
>   ui: fix crash on serial reset, during init (2023-03-21 11:46:22 +0400)
>
> ----------------------------------------------------------------
> ui/ fixes for 8.0
>
> ----------------------------------------------------------------
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/8.0
for any user-visible changes.

-- PMM