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

[Victor Toso]

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.

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

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

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

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/20181211/7559cae3/attachment.sig>