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

[Frediano Ziglio]

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

Why an API like:

gboolean 
spice_usb_device_manager_create_shared_cd(SpiceUsbDeviceManager *manager,
                                          const char *share_cd, GError **err);

cannot work and propagate the error?

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