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

Wed Dec 12 11:43:04 UTC 2018

> Hi,
> 
> On Mon, Dec 10, 2018 at 08:39:46AM -0500, Frediano Ziglio wrote:
> > > On Mon, Dec 10, 2018 at 07:42:35AM -0500, Frediano Ziglio wrote:
> > > > > 
> > > > > From: Victor Toso <me at victortoso.com>
> > > > > 
> > > > > Spice client should listen to SpiceMainChannel::agent-connected
> > > > > notification and avoid calling any clipboard related functions such
> > > > > as
> > > > > spice_gtk_session_paste_from_guest() from client-gtk library.
> > > > > 
> > > > > This patch removes the silent return of agent_clipboard_grab() in
> > > > > order to properly catch bugs with critical messages.
> > > > > 
> > > > > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > > > 
> > > > Documentation for agent-connected: "Whether the agent is connected"
> > > > 
> > > > Documentation for main-agent-update signal:
> > > > 
> > > > /**
> > > >  * SpiceMainChannel::main-agent-update:
> > > >  * @main: the #SpiceMainChannel that emitted the signal
> > > >  *
> > > >  * Notify when the %SpiceMainChannel:agent-connected or
> > > >  * %SpiceMainChannel:agent-caps-0 property change.
> > > >  **/
> > > > 
> > > > Documentation for spice_main_channel_clipboard_selection_grab
> > > > 
> > > > /**
> > > >  * spice_main_channel_clipboard_selection_grab:
> > > >  * @channel: a #SpiceMainChannel
> > > >  * @selection: one of the clipboard #VD_AGENT_CLIPBOARD_SELECTION_*
> > > >  * @types: an array of #VD_AGENT_CLIPBOARD types available in the
> > > >  clipboard
> > > >  * @ntypes: the number of @types
> > > >  *
> > > >  * Grab the guest clipboard, with #VD_AGENT_CLIPBOARD @types.
> > > >  *
> > > >  * Since: 0.35
> > > >  **/
> > > > 
> > > > Documentation for spice_gtk_session_paste_from_guest
> > > > 
> > > > /**
> > > >  * spice_gtk_session_paste_from_guest:
> > > >  * @self: #SpiceGtkSession
> > > >  *
> > > >  * Copy guest clipboard to client-side clipboard.
> > > >  *
> > > >  * Since 0.8
> > > >  **/
> > > > 
> > > > I don't see any requirement for the client to check agent status.
> > > 
> > > You mean that we should improve documentation or something else?
> > > 
> > 
> > I was expecting a "is documented here".
> 
> 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

> > 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.

> > 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.
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.
Also I think that a "critical" is a bit strong as they are used
also for programming error.

> 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
> > > 
> 