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

Mon Jul 29 07:48:28 UTC 2019

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

Frediano