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

[Yuri Benditovich]

On Mon, Jul 29, 2019 at 10:48 AM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> >
> > 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
> > > > > >
> >
>
> One step back. This hack was introduced for the "property" hack.
> This patchset if I'm not mistaken should implement all code necessary
> for spice-glib, that is the no-GUI part. At this point should already have
> a final API to add cd sharing but beside the the property hack I cannot
> see such interface. Is there another patch series before the UI one?
> In this case patches 2/9 and 3/9 of this current series should be moved
> there and all this discussion too.

This patch is _not_ one that uses property for creation of shared cd.
It is dedicated to returning error o the application regardless when
and why the error happens in each case when we do not have a device we
can refer.
The documentation states that device_error is for 'redirected' device.
First of all: why? The answer is - because until local USB device is
redirected, existing code (including libusb) does not have any
information of what happens with the device, it is informed only when
the device is physically removed.
When there is a problem, for example, with existing emulated device,
spice-gtk has all the information about it, whether it is redirected
or not.
When there is a problem with creation of emulated device, it also
would be good to raise event up to the application.
This can be done by defining special signal or using existing one if
this makes sense.
IMO, existing one is preferred way.

> The fact that you want to "propagate" the error from session to widget
> suggests me that there's other code for spice-glib (session is correctly
> in spice-glib) dealing with cd sharing. That is we are discussing here
> this new interface without code for it. Why session should not have an
> interface returning correctly the error? Is such interface implemented
> using a property hack too? Do you have code for this future interface?

I've never claimed I want (and do not have any plan to) propagate
error from session to widget.
I intentionally do not discuss now the future GUI and make an effort
to avoid defining interface suitable for GUI right now, as it takes us
to discussion about GUI and makes the discussion much more
complicated.
In the _text above_ Victor suggested that remote-viewer shall have
such GUI command, I've commented on that.
Currently only interface is via command-line, it makes sense to have
it in --spice* area, i.e. under 'spice-session' control.
Spice-session does not have any ability to raise error to the
application and I do not see any stong reason to invent it.
We already have everything we need with this small change.

> Frediano