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

[Yuri Benditovich]

On Fri, Jul 26, 2019 at 10:09 AM Victor Toso <victortoso at redhat.com> wrote:
>
> Hi,
>
> On Fri, Jul 26, 2019 at 08:08:07AM +0300, Yuri Benditovich wrote:
> > On Thu, Jul 25, 2019 at 8:46 PM Frediano Ziglio <fziglio at redhat.com> wrote:
> > > > > > > @@ -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).
>
> Heh, nice try. The difference here, IMO, would be that you have a
> clear objective: give an error when device creation fails. You
> want to do it by emit an error signal in a fake, empty device
> which is quite the workaround and I wouldn't call it a simpler
> alternative.
>
> > 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.
>
> That upsets me a little. When I started learning the usb stack in
> spice-gtk to give some though on the design proposals, I saw lots
> of potential to the usb-backend work. What you proposes here goes
> in opposite direction of a clear definition of what each
> component of this does.
>
> So, I'd say also in reply to your previous argument around not
> defining an API. We can define an API an still change it before
> the next release, that's ok. It is also ok to deprecate it in the
> next release if we feel we did it wrong. But let's do it in the
> right way, trying to achieve something easy to understand and
> maintain.

The point is that even we define the API (in old preview version of
cd-sharing it was)
the error that can happen during creation of device can not be propagated up.
The spice-session does not have such ability (if I'm not mistaken).
So it can only issue debug warning. But this is always possible also
without current patch.
usb-device-manager is able to send the error up to the application,
this is a reason why I wanted to use this method.

>
> >
> > > 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.
>
> Cheers,
> Victor