On Mon, Jul 29, 2019 at 3:34 PM Frediano Ziglio <fziglio@xxxxxxxxxx> 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 > > > 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@xxxxxxxxxx> wrote: > > > > > > > On Mon, Jul 29, 2019 at 12:27 PM Frediano Ziglio <fziglio@xxxxxxxxxx> > > > > wrote: > > > > > > > > > > > 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 > > > > > > > > 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 > > > > > > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel