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

[Frediano Ziglio]

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

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