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

[Frediano Ziglio]

> 
> On Thu, Aug 8, 2019 at 1:41 PM Frediano Ziglio <fziglio at 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,