[Spice-devel] [spice-gtk 2/9] usb-redir: device error signal without device object
Yuri Benditovich
yuri.benditovich at daynix.com
Mon Jul 29 09:46:46 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?
>
> > 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
More information about the Spice-devel
mailing list