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

[Yuri Benditovich]

On Mon, Jul 29, 2019 at 6:32 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> >
> > On Mon, Jul 29, 2019 at 3:34 PM Frediano Ziglio <fziglio at redhat.com> wrote:
> > >
> > > >
> > > > But this was always possible to print the error.
> > >
> > > No... I don't print the error, is up to the caller.
> >
> > But you're also the caller (in spice-options), aren't you?
> > The difference is that what I suggested comes to existing signal set
> > by the application (by remote viewer) and processed automatically
> > https://pagure.io/virt-viewer/blob/master/f/src/virt-viewer-session-spice.c#_301
> >
>
> In this case the signal won't be processed, spice_set_session_option is
> called in create_spice_session just after spice_session_new before
> the signal you are using is connected.
> Yes, this will probably work from the GUI because at that time the
> signal will be registered and that's the reason I asked how this
> was managed with the GUI.

It was indeed merge from old code, where device creation was also from GUI.
calling API "create-cd" with wrong file name returned error, but the
widget has nothing to do with this error.
After that the device was not added (no problem for anyone), the error
was reported via device-error and shown by the application.

> Still you are using quite some hack, me and Victor agreed on adding
> another signal to handle more generic errors, in the new signal you
> could document for instance that it must be registered much earlier
> to catch initialization errors.

> Maybe would be worth adding that signal to the session instead of
> the USB manager to handle any possible errors from the session,
> that would make sense for spice_set_session_option too.

Only case I see for now is wrong file path, but even in this single
case there is no other way to report that.
My personal opinion is that making changes in several places (spice
session, then the application etc) to make ideal solution for one
corner case (wrong file name on device creation) is not the best
investment of time.
(But I know that for perfectionists never take in account a time as a factor)

> But I still don't agree on SpiceUsbDevice and property hack.
> But this signal looks like a logging target more than properly error handling.

If you insist on API right now, the workaround with property is not
required, so we do not need to discuss it.

> The signal handler has no much options, what to do with an error
> from session or usb manager (even if this last is indeed less generic)?
> I mean, beside notifying the user, there's no much possibility to
> say continue or retry from the signal. A code that call a function and
> get an error has much more understand of what's going on.

The user should be somehow informed that the file name he typed is wrong.

>
> > >
> > > > IMO, the point is to raise it to remote-viewer GUI.
> > >
> > > If is a GUI the fprintf will be replaced by a MessageBox
> > > or similar, isn't it?
> > >
> > > > And this is simple with fake device and complicated other way.
> > > >
> > > > On Mon, Jul 29, 2019 at 2:14 PM Frediano Ziglio <fziglio at redhat.com>
> > > > wrote:
> > > > >
> > > > > > 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
> > > > > >
> > > >
> >