[Spice-devel] [spice-gtk v2 6/7] channel-main: clipboard grab: don't fail silently

Fri Dec 14 10:44:20 UTC 2018

> Hi,
> 
> On Wed, Dec 12, 2018 at 06:43:04AM -0500, Frediano Ziglio wrote:
> > > Documentation saying that this feature rely on agent seems to be
> > > lacking. e.g: grep for 'agent' on SpiceGtkSession
> > > 
> > > > As spice-gtk is a public library with an API speaking of "Spice
> > > > client" is not a good thing, should be Spice clientS, meaning
> > > > all possible clients and being ALL would mean even
> > > > unknown/proprietary ones.
> > > 
> > > Hm, I did not mention a specific client. My usage of 'Spice
> > > client' is meant to be generic. I have no problem in changing
> > > this to make you happy but I don't see why is wrong.
> > > 
> > 
> > With your patches and remove-viewer (1 minutes or so):
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:27:52.344:
> > agent_clipboard_grab: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:27:52.344:
> > agent_clipboard_grab: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:17.152:
> > agent_clipboard_release: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:17.153:
> > agent_clipboard_grab: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:17.638:
> > agent_clipboard_release: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:17.640:
> > agent_clipboard_grab: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:30.751:
> > agent_clipboard_release: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:30.752:
> > agent_clipboard_grab: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:32.672:
> > agent_clipboard_release: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:32.679:
> > agent_clipboard_grab: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:39.305:
> > agent_clipboard_release: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:39.307:
> > agent_clipboard_grab: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:40.929:
> > agent_clipboard_release: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:40.931:
> > agent_clipboard_grab: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:45.805:
> > agent_clipboard_release: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:45.806:
> > agent_clipboard_grab: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:48.843:
> > agent_clipboard_release: assertion 'c->agent_connected' failed
> > 
> > (remote-viewer:11191): GSpice-CRITICAL **: 11:28:48.847:
> > agent_clipboard_grab: assertion 'c->agent_connected' failed
> 
> Thanks! This is exactly what should be fixed (I mean, I still
> think my patches aren't wrong ;] ) ~ This also shows that my test
> was a bit bad.
> 

I don't think here there's right or wrong, just different opinion.
Looks like the only user of these APIs is spice-gtk.

> I have fix locally, I hope to send the patches before closing the
> laptop today.
> 
> > > > As all clients should respect API documentation you are allow to change
> > > > API behaviour where the behaviour was documented as not defined (for
> > > > instance free() behaviour was changed in respect to NULL from undefined
> > > > behaviour to ignored).
> > > 
> > > Because I'm verbose on commit log, doesn't mean that I changed
> > > the behavior? If any function dealing with clipboard is handled
> > > while the agent is disconnected, that's bug and some
> > > warning/critical should be hit. I'm making it explicit in
> > > agent_clipboard_*() functions.
> > > 
> > 
> > not talking about commit log, the status check.
> > Is perfectly fine if the check is done in the functions, for
> > instance has the advantage to be shared between all possible
> > clients so you don't have to keep the state consistent for all
> > possible operations. I think is pretty normal.  That's the same
> > reason free() is accepting now NULL ignoring it, remove the
> > effort to have a "if (p) free(p)" in a lot of places.
> > 
> > > > At least we should check that the "expected" clients (remote-viewer,
> > > > virt-viewer, virt-manager I would say) already have this behaviour
> > > > before changing API behaviour.
> > > 
> > > SpiceGtkSession's clipboard handling is automatic if
> > > auto-clipboard property set or when it is false, by using
> > > spice_gtk_session_paste_from_guest() and
> > > spice_gtk_session_copy_to_guest() functions.
> > > 
> > > "auto-clipboard" is preferred by gnome-boxes, virt-viewer,
> > > virt-manager. Spicy **testing tool** has menu to use both
> > > functions above but they are disabled when agent is disconnected.
> > > 
> > 
> > Don't get these comments.
> 
> Just that there are two ways to achieve the clipboard
> functionality, setting SpiceGtkSession::auto-clipboard to true is
> the preferred way. See:
> 
>     https://www.spice-space.org/api/spice-gtk/SpiceGtkSession.html#SpiceGtkSession--auto-clipboard
> 
> > 
> > > > Is only a warning change but does not help if the change is
> > > > producing hundred of warnings which would hide any possible
> > > > issue raised by warnings, not having the agent running is not
> > > > an impossible use case.
> > > 
> > > The warning would happen anyway. It should happen. I agree with
> > > lack of documentation.
> > > 
> > 
> > Without these patches remote-viewer does not throw any message
> > to me and IMHO not having the agent running is something that
> > should be supported and not throw messages continuously.
> 
> Yes, thanks again for testing.
> 
> > As a user I would prefer a GUI message once stating that the
> > client is not able to do clipboard at the moment as cannot
> > detect the agent running. This will help the user to fix the
> > issue.
> 
> I agree but the only way to do that today is for the clients to
> connect to SpiceMainChannel::agent-connected and provide some
> info. That should be enough I think, but
> 
> > Also I think that a "critical" is a bit strong as they
> > are used also for programming error.
> 
> My intention is to have critical as programming error indeed.
> I hope that my next version clarify that.
> 

If you are going to change APIs (as this patch is doing) you need
to change the users to match the new behaviour. As it seems that
the only user is spice-gtk you need to update it.
Personally I'm perfectly fine with the current behaviour.

> Cheers,
> Victor
> 
> > > Cheers,
> > > 
> > > > 
> > > > > As always, thanks for reviewing.
> > > > > 
> > > > > > 
> > > > > > > ---
> > > > > > >  src/channel-main.c | 4 +---
> > > > > > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/src/channel-main.c b/src/channel-main.c
> > > > > > > index 223043a..aa563d2 100644
> > > > > > > --- a/src/channel-main.c
> > > > > > > +++ b/src/channel-main.c
> > > > > > > @@ -1352,9 +1352,7 @@ static bool
> > > > > > > agent_clipboard_grab(SpiceMainChannel
> > > > > > > *channel, guint selection,
> > > > > > >      size_t size;
> > > > > > >      int i;
> > > > > > >  
> > > > > > > -    if (!c->agent_connected)
> > > > > > > -        return false;
> > > > > > > -
> > > > > > > +    g_return_val_if_fail(c->agent_connected, false);
> > > > > > >      g_return_val_if_fail(test_agent_cap(channel,
> > > > > > >      VD_AGENT_CAP_CLIPBOARD_BY_DEMAND), false);
> > > > > > >  
> > > > > > >      size = sizeof(VDAgentClipboardGrab) + sizeof(uint32_t) *
> > > > > > >      ntypes;
> > > > > > 
> > > > > > Frediano
> > > > > 
> > > 
> 