[Spice-devel] [PATCH spice-gtk 08/14] usb: remove useless device ref/unref

[Jonathon Jongsma]

Hm, I agree that the existing code does look very suspicious, but in general I think that ensuring that a ref is held for the duration of the async operation is probably a good idea.  From a very cursory investigation it looks like the spice_usb_device_manager_remove_dev() call path could be problematic. This ends up calling spice_win_usb_driver_uninstall() and then removing the device (via g_ptr_array_remove()), which could end up dropping the final ref on the object. Then you'd have a dead object in the uninstall_cb callback.  Granted, the existing code doesn't appear to be safe in this scenario either...


----- Original Message -----
> From: "Marc-André Lureau" <marcandre.lureau at gmail.com>
> To: spice-devel at freedesktop.org
> Sent: Wednesday, April 23, 2014 1:09:13 PM
> Subject: [Spice-devel] [PATCH spice-gtk 08/14] usb: remove useless device	ref/unref
> 
> A code doing an unref() on an object just before manipulating it looks
> horribly suspicious...
> ---
>  gtk/usb-device-manager.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
> index a505c19..af993aa 100644
> --- a/gtk/usb-device-manager.c
> +++ b/gtk/usb-device-manager.c
> @@ -1094,10 +1094,8 @@ static void
> spice_usb_device_manager_drv_install_cb(GObject *gobject,
>          g_error_free(err);
>      }
>  
> -    spice_usb_device_unref(device);
>      spice_usb_device_set_state(device, SPICE_USB_DEVICE_STATE_INSTALLED);
>  
> -    /* device is already ref'ed */
>      _spice_usb_device_manager_connect_device_async(self,
>                                                     device,
>                                                     cancellable,
> @@ -1122,7 +1120,6 @@ static void
> spice_usb_device_manager_drv_uninstall_cb(GObject *gobject,
>          g_clear_error(&err);
>      }
>  
> -    spice_usb_device_unref(cbinfo->device);
>      spice_usb_device_set_state(cbinfo->device, SPICE_USB_DEVICE_STATE_NONE);
>  
>      g_free(cbinfo);
> @@ -1490,7 +1487,7 @@ void
> spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>      installer = self->priv->installer;
>      cbinfo = g_new0(UsbInstallCbInfo, 1);
>      cbinfo->manager     = self;
> -    cbinfo->device      = spice_usb_device_ref(device);
> +    cbinfo->device      = device;
>      cbinfo->installer   = installer;
>      cbinfo->cancellable = cancellable;
>      cbinfo->callback    = callback;
> @@ -1566,7 +1563,7 @@ void
> spice_usb_device_manager_disconnect_device(SpiceUsbDeviceManager *self,
>      installer = self->priv->installer;
>      cbinfo = g_new0(UsbInstallCbInfo, 1);
>      cbinfo->manager     = self;
> -    cbinfo->device      = spice_usb_device_ref(device);
> +    cbinfo->device      = device;
>      cbinfo->installer   = installer;
>      cbinfo->cancellable = NULL;
>      cbinfo->callback    = NULL;
> --
> 1.8.5.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>