> On Mon, Jul 29, 2019 at 10:48 AM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> >
> > >
> > > On Fri, Jul 26, 2019 at 7:14 PM Victor Toso <victortoso@xxxxxxxxxx>
> > > 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@xxxxxxxxxx>
> > > > > wrote:
> > > > > >
> > > > > > >
> > > > > > > On Fri, Jul 26, 2019 at 10:09 AM Victor Toso
> > > > > > > <victortoso@xxxxxxxxxx>
> > > > > > > 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@xxxxxxxxxx>
> > > > > > > > > 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
> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel