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

[Yuri Benditovich]

On Thu, Jul 25, 2019 at 8:46 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> > >
> > > 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.

IMO, creating special entity for each case that is little different
from existing ones
is disrespect to Occam's principle (and several similar ones). In context of
'device error signal' the 'device' is something that can
referenced/dereferenced and
which name can be retrieved.

> This is not a device error, it's a device manager error.

We can view device manager as kind of device, then there is conflict.

> This is caused by wanting to use an interface (properties)
> that does not allow to return an error instead.

As any solution, this one has pros and cons. From my personal point of view,
it has significant pro (low cost of implementation) and does not have
any significant con.

> Frediano