diff mbox series

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

Message ID 20230320111412.1516419-4-marcandre.lureau@redhat.com
State New
Headers show
Series Fix Spice regression on win32 | expand

Commit Message

Marc-André Lureau March 20, 2023, 11:14 a.m. UTC 
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>
---
 ui/dbus.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Daniel P. Berrangé March 20, 2023, 12:17 p.m. UTC | #1 
On Mon, Mar 20, 2023 at 03:14:12PM +0400, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>  ui/dbus.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/ui/dbus.c b/ui/dbus.c
> index 0513de9918..5389ac493f 100644
> --- a/ui/dbus.c
> +++ b/ui/dbus.c
> @@ -304,9 +304,17 @@ dbus_display_add_client(int csock, Error **errp)
>          g_cancellable_cancel(dbus_display->add_client_cancellable);
>      }
>  
> +#ifdef WIN32
> +    csock = qemu_close_to_socket(csock);
> +#endif

Happens to work because 'int' and 'SOCKET' are the same, but I feel
like this is confusing to overload one variable for two different
purposes. This confusing code pattern is the only reason why the
method in patch 1 needs to return SOCKET. 

>      socket = g_socket_new_from_fd(csock, &err);
>      if (!socket) {
>          error_setg(errp, "Failed to setup D-Bus socket: %s", err->message);
> +#ifdef WIN32
> +        closesocket(csock);
> +#else
> +        close(csock);
> +#endif

IMHO it would be clearer to write it as

  #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 ...
  }

  #ifdef WIN32
  /* socket owns the SOCKET handle now, so release our osf handle */
  qemu_close_socket_osfhandle(csock);
  #endif

With regards,
Daniel
diff mbox series

Patch

diff --git a/ui/dbus.c b/ui/dbus.c
index 0513de9918..5389ac493f 100644
--- a/ui/dbus.c
+++ b/ui/dbus.c
@@ -304,9 +304,17 @@  dbus_display_add_client(int csock, Error **errp)
         g_cancellable_cancel(dbus_display->add_client_cancellable);
     }
 
+#ifdef WIN32
+    csock = qemu_close_to_socket(csock);
+#endif
     socket = g_socket_new_from_fd(csock, &err);
     if (!socket) {
         error_setg(errp, "Failed to setup D-Bus socket: %s", err->message);
+#ifdef WIN32
+        closesocket(csock);
+#else
+        close(csock);
+#endif
         return false;
     }