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

Submitted by Victor Toso on Dec. 10, 2018, 11:02 a.m.

Details

Message ID 20181210110256.20115-7-victortoso@redhat.com
State New
Headers show
Series "small code improvements on clipboard" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Dec. 10, 2018, 11:02 a.m.
From: Victor Toso <me@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@redhat.com>
---
 src/channel-main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Patch hide | download patch | download mbox

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;

Comments

> 
> From: Victor Toso <me@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@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.

> ---
>  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
On Mon, Dec 10, 2018 at 07:42:35AM -0500, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me@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@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?

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
> On Mon, Dec 10, 2018 at 07:42:35AM -0500, Frediano Ziglio wrote:
> > > 
> > > From: Victor Toso <me@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@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
>
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@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@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
> >
> 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@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@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
> > > 
>
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
> > > > 
> >
> 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
> > > > > 
> > > 
>