[Spice-devel] [spice-gtk 2/9] usb-redir: device error signal without device object

[Frediano Ziglio]

> > 
> > On Thu, Jul 25, 2019 at 12:01 PM Frediano Ziglio <fziglio at redhat.com>
> > wrote:
> > >
> > > >
> > > > Add ability to indicate error to external modules via
> > > > 'device error' signal when real SpiceUsbDevice is not passed.
> > > > This is needed to indicate error during creation of emulated
> > > > device, when the device is not created yet. For that we
> > > > allocate temporary SpiceUsbDevice structure with backend
> > > > device fields set to NULL and use it for indication. Device
> > > > description for such device will be 'USB Redirection'.
> > > > Unreferencing of such device will be 'no operation'.
> > > >
> > > > Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> > > > ---
> > > >  src/usb-backend.c        |  3 +++
> > > >  src/usb-device-manager.c | 14 ++++++++++++--
> > > >  2 files changed, 15 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/usb-backend.c b/src/usb-backend.c
> > > > index 0bf2ecc..c179188 100644
> > > > --- a/src/usb-backend.c
> > > > +++ b/src/usb-backend.c
> > > > @@ -528,6 +528,9 @@ SpiceUsbBackendDevice
> > > > *spice_usb_backend_device_ref(SpiceUsbBackendDevice *dev)
> > > >
> > > >  void spice_usb_backend_device_unref(SpiceUsbBackendDevice *dev)
> > > >  {
> > > > +    if (!dev) {
> > > > +        return;
> > > > +    }
> > > >      LOUD_DEBUG("%s >> %p(%d)", __FUNCTION__, dev, dev->ref_count);
> > > >      if (g_atomic_int_dec_and_test(&dev->ref_count)) {
> > > >          if (dev->libusb_device) {
> > > > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > > > index a530be9..0961d16 100644
> > > > --- a/src/usb-device-manager.c
> > > > +++ b/src/usb-device-manager.c
> > > > @@ -935,10 +935,16 @@ static void
> > > > spice_usb_device_manager_check_redir_on_connect(
> > > >  void spice_usb_device_manager_device_error(
> > > >      SpiceUsbDeviceManager *self, SpiceUsbDevice *device, GError *err)
> > > >  {
> > > > +    SpiceUsbDevice *dev = device;
> > >
> > > "dev" and "device" do not sound great. Maybe "temp_dev" or "fake_dev" ?
> > >
> > > >      g_return_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self));
> > > > -    g_return_if_fail(device != NULL);
> > > > -
> > > > +    if (device == NULL) {
> > > > +        dev = g_new0(SpiceUsbDevice, 1);
> > > > +        dev->ref = 1;
> > > > +    }
> > > >      g_signal_emit(self, signals[DEVICE_ERROR], 0, device, err);
> > >
> > > If device was initially NULL at function call this is still NULL.
> > 
> > Sorry, typo.
> > 
> > >
> > > > +    if (device == NULL) {
> > > > +        spice_usb_device_unref(dev);
> > >
> > > If device was NULL you allocate a new empty SpiceUsbDevice and
> > > then free it. Not sure is what you want to do.
> > >
> > > > +    }
> 
> I would then rewrite as
> 
> {
>     SpiceUsbDevice *dummy_device = NULL;
>     g_return_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self));
> 
>     if (device == NULL) {
>         dummy_device = g_new0(SpiceUsbDevice, 1);
>         dummy_device->ref = 1;
>         device = dummy_device;
>     }
>     g_signal_emit(self, signals[DEVICE_ERROR], 0, device, err);
>     if (dummy_device != NULL) {
>         spice_usb_device_unref(dummy_device);
>     }
> }
> 
> > > >  }
> > > >  #endif
> > > >
> > > > @@ -1440,6 +1446,10 @@ gchar
> > > > *spice_usb_device_get_description(SpiceUsbDevice
> > > > *device, const gchar *for
> > > >
> > > >      g_return_val_if_fail(device != NULL, NULL);
> > > >
> > > > +    if (!device->bdev) {
> > > > +        return g_strdup(_("USB redirection"));
> > > > +    }
> > > > +
> > > >      bus     = spice_usb_device_get_busnum(device);
> > > >      address = spice_usb_device_get_devaddr(device);
> > > >      vid     = spice_usb_device_get_vid(device);
> > >

Ok, now I had understand this patch. This is removing the
assumption that bdev is never NULL.
Only to support calling spice_usb_device_manager_device_error
with a NULL device.
I would say nack to this patch and find another solution.
Maybe adding a "device_creation_error" signal with "error"
but no device.
This is not a device error, it's a device manager error.
This is caused by wanting to use an interface (properties)
that does not allow to return an error instead.

Frediano