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

[Victor Toso]

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.

> 
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190726/a15654ed/attachment-0001.sig>