[spice-gtk] fixup! usb-redir: define interfaces to support emulated devices

Submitted by Frediano Ziglio on Aug. 8, 2019, 10:40 a.m.

Details

Message ID 20190808104053.11449-1-fziglio@redhat.com
State Superseded
Headers show
Series "fixup! usb-redir: define interfaces to support emulated devices" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Aug. 8, 2019, 10:40 a.m.
Change reference counting semantic for emulated devices.
Make the same as not emulated.
This fix a leak if spice_usb_backend_device_eject is not called.
Consistently the reference counter for SpiceUsbBackendDevice is
now the number of owning pointers.
---
 src/usb-backend.c   | 19 ++++++++++---------
 src/usb-emulation.h |  2 +-
 2 files changed, 11 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/usb-backend.c b/src/usb-backend.c
index d80da177..de2b55ec 100644
--- a/src/usb-backend.c
+++ b/src/usb-backend.c
@@ -852,17 +852,17 @@  void spice_usb_backend_device_report_change(SpiceUsbBackend *be,
 
 void spice_usb_backend_device_eject(SpiceUsbBackend *be, SpiceUsbBackendDevice *dev)
 {
-    g_return_if_fail(dev && dev->edev);
+    g_return_if_fail(dev);
 
+    if (dev->edev) {
+        be->own_devices_mask &= ~(1 << dev->device_info.address);
+    }
     if (be->hotplug_callback) {
         be->hotplug_callback(be->hotplug_user_data, dev, FALSE);
     }
-    be->own_devices_mask &= ~(1 << dev->device_info.address);
-
-    spice_usb_backend_device_unref(dev);
 }
 
-SpiceUsbBackendDevice*
+gboolean
 spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
                                          SpiceUsbEmulatedDeviceCreate create_proc,
                                          void *create_params,
@@ -877,7 +877,7 @@  spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
     if (be->own_devices_mask == 0xffffffff) {
         g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
                     _("can't create device - limit reached"));
-        return NULL;
+        return FALSE;
     }
     for (address = 0; address < 32; ++address) {
         if (~be->own_devices_mask & (1 << address)) {
@@ -893,7 +893,7 @@  spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
     dev->edev = edev = create_proc(be, dev, create_params, err);
     if (edev == NULL) {
         spice_usb_backend_device_unref(dev);
-        return NULL;
+        return FALSE;
     }
 
     if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_DEVICE, 0,
@@ -903,7 +903,7 @@  spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
         spice_usb_backend_device_unref(dev);
         g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
                     _("can't create device - internal error"));
-        return NULL;
+        return FALSE;
     }
 
     be->own_devices_mask |= 1 << address;
@@ -918,8 +918,9 @@  spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
     if (be->hotplug_callback) {
         be->hotplug_callback(be->hotplug_user_data, dev, TRUE);
     }
+    spice_usb_backend_device_unref(dev);
 
-    return dev;
+    return TRUE;
 }
 
 #endif /* USB_REDIR */
diff --git a/src/usb-emulation.h b/src/usb-emulation.h
index 9e626a24..46d54d47 100644
--- a/src/usb-emulation.h
+++ b/src/usb-emulation.h
@@ -80,7 +80,7 @@  static inline const UsbDeviceOps *device_ops(SpiceUsbEmulatedDevice *dev)
     return (const UsbDeviceOps *)dev;
 }
 
-SpiceUsbBackendDevice*
+gboolean
 spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
                                          SpiceUsbEmulatedDeviceCreate create_proc,
                                          void *create_params,

Comments

On Thu, Aug 8, 2019 at 1:41 PM Frediano Ziglio <fziglio@redhat.com> wrote:
>
> Change reference counting semantic for emulated devices.
> Make the same as not emulated.
> This fix a leak if spice_usb_backend_device_eject is not called.
> Consistently the reference counter for SpiceUsbBackendDevice is
> now the number of owning pointers.
> ---
>  src/usb-backend.c   | 19 ++++++++++---------
>  src/usb-emulation.h |  2 +-
>  2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/src/usb-backend.c b/src/usb-backend.c
> index d80da177..de2b55ec 100644
> --- a/src/usb-backend.c
> +++ b/src/usb-backend.c
> @@ -852,17 +852,17 @@ void spice_usb_backend_device_report_change(SpiceUsbBackend *be,
>
>  void spice_usb_backend_device_eject(SpiceUsbBackend *be, SpiceUsbBackendDevice *dev)
>  {
> -    g_return_if_fail(dev && dev->edev);
> +    g_return_if_fail(dev);
>
> +    if (dev->edev) {
> +        be->own_devices_mask &= ~(1 << dev->device_info.address);
> +    }
>      if (be->hotplug_callback) {
>          be->hotplug_callback(be->hotplug_user_data, dev, FALSE);
>      }
> -    be->own_devices_mask &= ~(1 << dev->device_info.address);
> -
> -    spice_usb_backend_device_unref(dev);
>  }
>
> -SpiceUsbBackendDevice*
> +gboolean
>  spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
>                                           SpiceUsbEmulatedDeviceCreate create_proc,
>                                           void *create_params,
> @@ -877,7 +877,7 @@ spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
>      if (be->own_devices_mask == 0xffffffff) {
>          g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
>                      _("can't create device - limit reached"));
> -        return NULL;
> +        return FALSE;
>      }
>      for (address = 0; address < 32; ++address) {
>          if (~be->own_devices_mask & (1 << address)) {
> @@ -893,7 +893,7 @@ spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
>      dev->edev = edev = create_proc(be, dev, create_params, err);
>      if (edev == NULL) {
>          spice_usb_backend_device_unref(dev);
> -        return NULL;
> +        return FALSE;
>      }
>
>      if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_DEVICE, 0,
> @@ -903,7 +903,7 @@ spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
>          spice_usb_backend_device_unref(dev);
>          g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
>                      _("can't create device - internal error"));
> -        return NULL;
> +        return FALSE;
>      }
>
>      be->own_devices_mask |= 1 << address;
> @@ -918,8 +918,9 @@ spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
>      if (be->hotplug_callback) {
>          be->hotplug_callback(be->hotplug_user_data, dev, TRUE);
>      }
> +    spice_usb_backend_device_unref(dev);

I was thinking about doing that... What looks for me a little
problematic in such solution is that cd-device keeps a pointer to the
backend device, but this device is referenced only by
usb-device-manager and never referenced by anybody at backend area.
If you're ok with this fixup, let's use it. I see it working in all
functional flows and in live migration.

>
> -    return dev;
> +    return TRUE;
>  }
>
>  #endif /* USB_REDIR */
> diff --git a/src/usb-emulation.h b/src/usb-emulation.h
> index 9e626a24..46d54d47 100644
> --- a/src/usb-emulation.h
> +++ b/src/usb-emulation.h
> @@ -80,7 +80,7 @@ static inline const UsbDeviceOps *device_ops(SpiceUsbEmulatedDevice *dev)
>      return (const UsbDeviceOps *)dev;
>  }
>
> -SpiceUsbBackendDevice*
> +gboolean
>  spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
>                                           SpiceUsbEmulatedDeviceCreate create_proc,
>                                           void *create_params,
> --
> 2.20.1
>
> 
> On Thu, Aug 8, 2019 at 1:41 PM Frediano Ziglio <fziglio@redhat.com> wrote:
> >
> > Change reference counting semantic for emulated devices.
> > Make the same as not emulated.
> > This fix a leak if spice_usb_backend_device_eject is not called.
> > Consistently the reference counter for SpiceUsbBackendDevice is
> > now the number of owning pointers.
> > ---
> >  src/usb-backend.c   | 19 ++++++++++---------
> >  src/usb-emulation.h |  2 +-
> >  2 files changed, 11 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/usb-backend.c b/src/usb-backend.c
> > index d80da177..de2b55ec 100644
> > --- a/src/usb-backend.c
> > +++ b/src/usb-backend.c
> > @@ -852,17 +852,17 @@ void
> > spice_usb_backend_device_report_change(SpiceUsbBackend *be,
> >
> >  void spice_usb_backend_device_eject(SpiceUsbBackend *be,
> >  SpiceUsbBackendDevice *dev)
> >  {
> > -    g_return_if_fail(dev && dev->edev);
> > +    g_return_if_fail(dev);
> >
> > +    if (dev->edev) {
> > +        be->own_devices_mask &= ~(1 << dev->device_info.address);
> > +    }
> >      if (be->hotplug_callback) {
> >          be->hotplug_callback(be->hotplug_user_data, dev, FALSE);
> >      }
> > -    be->own_devices_mask &= ~(1 << dev->device_info.address);
> > -
> > -    spice_usb_backend_device_unref(dev);
> >  }
> >
> > -SpiceUsbBackendDevice*
> > +gboolean
> >  spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
> >                                           SpiceUsbEmulatedDeviceCreate
> >                                           create_proc,
> >                                           void *create_params,
> > @@ -877,7 +877,7 @@
> > spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
> >      if (be->own_devices_mask == 0xffffffff) {
> >          g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> >                      _("can't create device - limit reached"));
> > -        return NULL;
> > +        return FALSE;
> >      }
> >      for (address = 0; address < 32; ++address) {
> >          if (~be->own_devices_mask & (1 << address)) {
> > @@ -893,7 +893,7 @@
> > spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
> >      dev->edev = edev = create_proc(be, dev, create_params, err);
> >      if (edev == NULL) {
> >          spice_usb_backend_device_unref(dev);
> > -        return NULL;
> > +        return FALSE;
> >      }
> >
> >      if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_DEVICE, 0,
> > @@ -903,7 +903,7 @@
> > spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
> >          spice_usb_backend_device_unref(dev);
> >          g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> >                      _("can't create device - internal error"));
> > -        return NULL;
> > +        return FALSE;
> >      }
> >
> >      be->own_devices_mask |= 1 << address;
> > @@ -918,8 +918,9 @@
> > spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
> >      if (be->hotplug_callback) {
> >          be->hotplug_callback(be->hotplug_user_data, dev, TRUE);
> >      }
> > +    spice_usb_backend_device_unref(dev);
> 
> I was thinking about doing that... What looks for me a little
> problematic in such solution is that cd-device keeps a pointer to the
> backend device, but this device is referenced only by
> usb-device-manager and never referenced by anybody at backend area.
> If you're ok with this fixup, let's use it. I see it working in all
> functional flows and in live migration.
> 

Making the life of the device longer (as it was before this fixup)
does not make things better in this respect but worse, the "backend"
pointer in SpiceUsbEmulatedDevice will be invalid (pointing to garbage)
if the device outlive the backend.
I would use a weak pointer but the backend is not implementing them.
Adding a strong reference can cause the backend to outlive the usb manager
which seems even worst.

> >
> > -    return dev;
> > +    return TRUE;
> >  }
> >
> >  #endif /* USB_REDIR */
> > diff --git a/src/usb-emulation.h b/src/usb-emulation.h
> > index 9e626a24..46d54d47 100644
> > --- a/src/usb-emulation.h
> > +++ b/src/usb-emulation.h
> > @@ -80,7 +80,7 @@ static inline const UsbDeviceOps
> > *device_ops(SpiceUsbEmulatedDevice *dev)
> >      return (const UsbDeviceOps *)dev;
> >  }
> >
> > -SpiceUsbBackendDevice*
> > +gboolean
> >  spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
> >                                           SpiceUsbEmulatedDeviceCreate
> >                                           create_proc,
> >                                           void *create_params,