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

[Yuri Benditovich]

On Fri, Jul 26, 2019 at 7:14 PM Victor Toso <victortoso at redhat.com> wrote:
>
> 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.

I do not see any reason to involve remote-viewer to creation of CD.
I think this is a way to write 10x more code than required.
>From my point of view, this depends on usb-redirection and should be
in the widget of usb-redir.
But this is too early to discuss the UI.
Command-line creation of shared-cd is also function of remote viewer?

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

And why changing the documentation is impossible?
As I can see nobody uses the fact that device error is emitted for
redirected device.

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