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

[Yuri Benditovich]

On Fri, Jul 26, 2019 at 2:09 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> >
> > 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?

Tell me if I am mistaken.
Propagate (for me) means report the error to the application where it
can be processed, as it is done with device_error.

If spice-session-something calls
spice_usb_device_manager_create_shared_cd and receive error,
it does not have a way to raise this indication to the viever, need to
invent it.
So, IMO, even with API it is preferred way to use device_error with fake device.
BTW, the remote-viewer ignore the device parameter and uses only error->message

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