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

[Daniel P . Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|