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

[Yuri Benditovich]

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.

>
> -    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
>