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

[Victor Toso]

Hi,

On Fri, Jul 26, 2019 at 06:30:51PM +0300, Yuri Benditovich wrote:
> 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.

Yes, let's say, remote-viewer's UI has somewhere "Redirect CD"
which opens a file chooser so you can browse and find that ISO
you want. If, for some reason redirect cd fails, an error should
pop up somewhere. Ok.

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

Yes..

> 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

.. and yes. I understand what you mean but for a second, let's
focus on the documentation of "device-error"

    "The #SpiceUsbDeviceManager::device-error signal is emitted
     whenever an error happens which causes a device to no longer
     be available to the guest."

It was redirect but then, not anymore. Not the case here, thus a
workaround.

So, doing what you propose is easier but gets a bit confusing
with existing code and its purposes. I'd either have a new way to
cover both cases (likely deprecating device-error for instance)
or something specific to this emulation code, e.g:
emulation-error, and yes, add that to all spice clients that are
interested in cd-room.

That's how I see this at the moment.

> > > > > > 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/398166b3/attachment.sig>