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

Wed Dec 12 15:11:59 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 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.

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
> > > > 
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20181212/c36ab7ae/attachment.sig>