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

Mon Jul 29 11:14:25 UTC 2019

> On Mon, Jul 29, 2019 at 12:27 PM Frediano Ziglio <fziglio at redhat.com> wrote:
> >
> > > 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.
> >
> > Yes, but is used only by 3/9 so the comment.
> >
> > > 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.
> >
> > Yes, and my proper API had this possibility without using hacks
> 
> I do not see how your proper API can do that. Can you explain, please?
> I see that it is able to collect the GError (my improper way of device
> creation also can do that).
> But how your API is better in propagating the error up?
> 

Like this:
https://cgit.freedesktop.org/~fziglio/spice-gtk/commit/?h=yuri_propagate&id=dfd0e45de457a6ca83637b549c0aa62197ebedb6

It works for me, I can see the error with my formatting instead of just the warning.

> >
> > > This can be done by defining special signal or using existing one if
> > > this makes sense.
> >
> > Or using a proper API instead of the property hack
> >
> > > IMO, existing one is preferred way.
> > >
> >
> > IMO proper API is better than the property hack
> >
> > > > 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.
> >
> > You used the fact that session could not propagate the error as a reason
> > for this hack.
> >
> > > 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.
> >
> > We are not defining the GUI but a spice-glib API to add cd sharing and
> > returning an error. This the above statement I assume the current API
> > you are proposing is the property hack and I don't agree.
> >
> > > In the _text above_ Victor suggested that remote-viewer shall have
> > > such GUI command, I've commented on that.
> >
> > I think Victor was referring as remote-viewer as a application, not
> > looking at the various component. So I think it's right, whatever GUI
> > application will have a GUI (in either spice-gtk or part of the application
> > component) to do it. But Victor will probably clarify this.
> >
> > > Currently only interface is via command-line, it makes sense to have
> > > it in --spice* area, i.e. under 'spice-session' control.
> >
> > Fine, but command-line interface is currently using the property API
> > to communicate with spice-glib that become an API.
> >
> > > Spice-session does not have any ability to raise error to the
> > > application and I do not see any stong reason to invent it.
> >
> > And I don't see the reason so add it and the reason of this patch.
> >
> > > We already have everything we need with this small change.
> > >
> >
> > We already have everything even without this patch, just changing
> > API.
> >
> > Frediano
> 