[spice-gtk,v1,1/4] gtk-session: remove extra clipboard selection check

Submitted by Victor Toso on Dec. 5, 2018, 3:52 p.m.

Details

Message ID 20181205155244.19933-2-victortoso@redhat.com
State New
Headers show
Series "gtk-session misc changes" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Dec. 5, 2018, 3:52 p.m.
From: Victor Toso <me@victortoso.com>

Commit 284c1f2d switched to
spice_main_channel_clipboard_selection_release() which does check if
agent is connected and does the right thing (expected) in regards to
releasing the clipboard by calling agent_clipboard_release() which
does check VD_AGENT_CAP_CLIPBOARD_SELECTION (like current removed
code).

So this patch removes redundant check.

Same goes for spice_main_channel_clipboard_selection_grab() function.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/spice-gtk-session.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 20a4060..8d31045 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -610,9 +610,7 @@  static void clipboard_get_targets(GtkClipboard *clipboard,
     }
 
     s->clip_grabbed[selection] = TRUE;
-
-    if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
-        spice_main_channel_clipboard_selection_grab(s->main, selection, types, num_types);
+    spice_main_channel_clipboard_selection_grab(s->main, selection, types, num_types);
 
     /* Sending a grab causes the agent to do an implicit release */
     s->nclip_targets[selection] = 0;
@@ -636,8 +634,7 @@  static void clipboard_owner_change(GtkClipboard        *clipboard,
 
     if (s->clip_grabbed[selection]) {
         s->clip_grabbed[selection] = FALSE;
-        if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
-            spice_main_channel_clipboard_selection_release(s->main, selection);
+        spice_main_channel_clipboard_selection_release(s->main, selection);
     }
 
     switch (event->reason) {

Comments

> 
> From: Victor Toso <me@victortoso.com>
> 
> Commit 284c1f2d switched to
> spice_main_channel_clipboard_selection_release() which does check if
> agent is connected and does the right thing (expected) in regards to
> releasing the clipboard by calling agent_clipboard_release() which
> does check VD_AGENT_CAP_CLIPBOARD_SELECTION (like current removed
> code).
> 
> So this patch removes redundant check.
> 
> Same goes for spice_main_channel_clipboard_selection_grab() function.
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>

This is not the same. The test in agent_clipboard_grab is done with
g_return_if_fail which emits a warning while current code does not.
Also spice_main_channel_clipboard_selection_grab always calls
spice_channel_wakeup which current code does not.

> ---
>  src/spice-gtk-session.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 20a4060..8d31045 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -610,9 +610,7 @@ static void clipboard_get_targets(GtkClipboard
> *clipboard,
>      }
>  
>      s->clip_grabbed[selection] = TRUE;
> -
> -    if (spice_main_channel_agent_test_capability(s->main,
> VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> -        spice_main_channel_clipboard_selection_grab(s->main, selection,
> types, num_types);
> +    spice_main_channel_clipboard_selection_grab(s->main, selection, types,
> num_types);
>  
>      /* Sending a grab causes the agent to do an implicit release */
>      s->nclip_targets[selection] = 0;
> @@ -636,8 +634,7 @@ static void clipboard_owner_change(GtkClipboard
> *clipboard,
>  
>      if (s->clip_grabbed[selection]) {
>          s->clip_grabbed[selection] = FALSE;
> -        if (spice_main_channel_agent_test_capability(s->main,
> VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> -            spice_main_channel_clipboard_selection_release(s->main,
> selection);
> +        spice_main_channel_clipboard_selection_release(s->main, selection);
>      }
>  
>      switch (event->reason) {

Frediano
Hi,

On Thu, Dec 06, 2018 at 03:54:41AM -0500, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me@victortoso.com>
> > 
> > Commit 284c1f2d switched to
> > spice_main_channel_clipboard_selection_release() which does check if
> > agent is connected and does the right thing (expected) in regards to
> > releasing the clipboard by calling agent_clipboard_release() which
> > does check VD_AGENT_CAP_CLIPBOARD_SELECTION (like current removed
> > code).
> > 
> > So this patch removes redundant check.
> > 
> > Same goes for spice_main_channel_clipboard_selection_grab() function.
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> 
> This is not the same. The test in agent_clipboard_grab is done
> with g_return_if_fail which emits a warning while current code
> does not.

Which probably should

> Also spice_main_channel_clipboard_selection_grab always calls
> spice_channel_wakeup which current code does not.

Which is probably a bug there (should call spice_channel_wakeup
if message was queued.

What do you think?

> > ---
> >  src/spice-gtk-session.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 20a4060..8d31045 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -610,9 +610,7 @@ static void clipboard_get_targets(GtkClipboard
> > *clipboard,
> >      }
> >  
> >      s->clip_grabbed[selection] = TRUE;
> > -
> > -    if (spice_main_channel_agent_test_capability(s->main,
> > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> > -        spice_main_channel_clipboard_selection_grab(s->main, selection,
> > types, num_types);
> > +    spice_main_channel_clipboard_selection_grab(s->main, selection, types,
> > num_types);
> >  
> >      /* Sending a grab causes the agent to do an implicit release */
> >      s->nclip_targets[selection] = 0;
> > @@ -636,8 +634,7 @@ static void clipboard_owner_change(GtkClipboard
> > *clipboard,
> >  
> >      if (s->clip_grabbed[selection]) {
> >          s->clip_grabbed[selection] = FALSE;
> > -        if (spice_main_channel_agent_test_capability(s->main,
> > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> > -            spice_main_channel_clipboard_selection_release(s->main,
> > selection);
> > +        spice_main_channel_clipboard_selection_release(s->main, selection);
> >      }
> >  
> >      switch (event->reason) {
> 
> Frediano
> Hi,
> 
> On Thu, Dec 06, 2018 at 03:54:41AM -0500, Frediano Ziglio wrote:
> > > 
> > > From: Victor Toso <me@victortoso.com>
> > > 
> > > Commit 284c1f2d switched to
> > > spice_main_channel_clipboard_selection_release() which does check if
> > > agent is connected and does the right thing (expected) in regards to
> > > releasing the clipboard by calling agent_clipboard_release() which
> > > does check VD_AGENT_CAP_CLIPBOARD_SELECTION (like current removed
> > > code).
> > > 
> > > So this patch removes redundant check.
> > > 
> > > Same goes for spice_main_channel_clipboard_selection_grab() function.
> > > 
> > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > 
> > This is not the same. The test in agent_clipboard_grab is done
> > with g_return_if_fail which emits a warning while current code
> > does not.
> 
> Which probably should
> 

IMHO is the opposite, as we supports agents that does not support
VD_AGENT_CAP_CLIPBOARD_BY_DEMAND why giving warning every time?
If we would like to warn the user that guest is using a very old
agent would not be better to warn once when the capabilities are
received? Maybe if it's really important this could be also done
with a GUI message (maybe with a persistent tick "don't warn
about it next time" like for the exit message).

> > Also spice_main_channel_clipboard_selection_grab always calls
> > spice_channel_wakeup which current code does not.
> 
> Which is probably a bug there (should call spice_channel_wakeup
> if message was queued.
> 
> What do you think?
> 

Not sure if this function is also expected to handle pending
events and what would happen if this is removed.
If it's safe to do so then I would move spice_channel_wakeup
call inside spice_main_channel_clipboard_selection_grab at
the end of agent_clipboard_grab (which is only called by
spice_main_channel_clipboard_selection_grab).

> > > ---
> > >  src/spice-gtk-session.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > index 20a4060..8d31045 100644
> > > --- a/src/spice-gtk-session.c
> > > +++ b/src/spice-gtk-session.c
> > > @@ -610,9 +610,7 @@ static void clipboard_get_targets(GtkClipboard
> > > *clipboard,
> > >      }
> > >  
> > >      s->clip_grabbed[selection] = TRUE;
> > > -
> > > -    if (spice_main_channel_agent_test_capability(s->main,
> > > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> > > -        spice_main_channel_clipboard_selection_grab(s->main, selection,
> > > types, num_types);
> > > +    spice_main_channel_clipboard_selection_grab(s->main, selection,
> > > types,
> > > num_types);
> > >  
> > >      /* Sending a grab causes the agent to do an implicit release */
> > >      s->nclip_targets[selection] = 0;
> > > @@ -636,8 +634,7 @@ static void clipboard_owner_change(GtkClipboard
> > > *clipboard,
> > >  
> > >      if (s->clip_grabbed[selection]) {
> > >          s->clip_grabbed[selection] = FALSE;
> > > -        if (spice_main_channel_agent_test_capability(s->main,
> > > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> > > -            spice_main_channel_clipboard_selection_release(s->main,
> > > selection);
> > > +        spice_main_channel_clipboard_selection_release(s->main,
> > > selection);
> > >      }
> > >  
> > >      switch (event->reason) {
> > 
> > Frediano
>
Hi,

On Thu, Dec 06, 2018 at 04:30:17AM -0500, Frediano Ziglio wrote:
> > Hi,
> > 
> > On Thu, Dec 06, 2018 at 03:54:41AM -0500, Frediano Ziglio wrote:
> > > > 
> > > > From: Victor Toso <me@victortoso.com>
> > > > 
> > > > Commit 284c1f2d switched to
> > > > spice_main_channel_clipboard_selection_release() which does check if
> > > > agent is connected and does the right thing (expected) in regards to
> > > > releasing the clipboard by calling agent_clipboard_release() which
> > > > does check VD_AGENT_CAP_CLIPBOARD_SELECTION (like current removed
> > > > code).
> > > > 
> > > > So this patch removes redundant check.
> > > > 
> > > > Same goes for spice_main_channel_clipboard_selection_grab() function.
> > > > 
> > > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > > 
> > > This is not the same. The test in agent_clipboard_grab is done
> > > with g_return_if_fail which emits a warning while current code
> > > does not.
> > 
> > Which probably should
> > 
> 
> IMHO is the opposite, as we supports agents that does not
> support VD_AGENT_CAP_CLIPBOARD_BY_DEMAND why giving warning
> every time?

Hm, that was added in 2010 [0], spice-vdagent-0.6.3. Why should
we support supper earlier versions of the agent anyway?

https://gitlab.freedesktop.org/spice/linux/vd_agent/commit/653a2ac191dd699de7e9dd322e4b#cbc5e2eb746c0c8aaa6d4629e43dfb9da6b0c945_94_67

> If we would like to warn the user that guest is using a very old
> agent would not be better to warn once when the capabilities are
> received? Maybe if it's really important this could be also done
> with a GUI message (maybe with a persistent tick "don't warn
> about it next time" like for the exit message).

I'm pretty sure nobody tests such old agent... I'm in favor of
warning as it should help find bugs instead of silently not doing
it like current code.

> > > Also spice_main_channel_clipboard_selection_grab always calls
> > > spice_channel_wakeup which current code does not.
> > 
> > Which is probably a bug there (should call spice_channel_wakeup
> > if message was queued.
> > 
> > What do you think?
> > 
> 
> Not sure if this function is also expected to handle pending
> events and what would happen if this is removed.

No, the wakeup is after adding the message to the queue with the
purpose of not delaying that message to be sent. Some code in
client could block while waiting the clipboard request and that's
why the wakeup can be helpful.

> If it's safe to do so then I would move spice_channel_wakeup
> call inside spice_main_channel_clipboard_selection_grab at
> the end of agent_clipboard_grab (which is only called by
> spice_main_channel_clipboard_selection_grab).

I would rather make agent_clipboard_* to return true if
succeed in adding the message to the queue. That's implementation
detail, We/I can look later on.. Important is that we agree
wakeup shouldn't be called on failure.

> > > > ---
> > > >  src/spice-gtk-session.c | 7 ++-----
> > > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > > index 20a4060..8d31045 100644
> > > > --- a/src/spice-gtk-session.c
> > > > +++ b/src/spice-gtk-session.c
> > > > @@ -610,9 +610,7 @@ static void clipboard_get_targets(GtkClipboard
> > > > *clipboard,
> > > >      }
> > > >  
> > > >      s->clip_grabbed[selection] = TRUE;
> > > > -
> > > > -    if (spice_main_channel_agent_test_capability(s->main,
> > > > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> > > > -        spice_main_channel_clipboard_selection_grab(s->main, selection,
> > > > types, num_types);
> > > > +    spice_main_channel_clipboard_selection_grab(s->main, selection,
> > > > types,
> > > > num_types);
> > > >  
> > > >      /* Sending a grab causes the agent to do an implicit release */
> > > >      s->nclip_targets[selection] = 0;
> > > > @@ -636,8 +634,7 @@ static void clipboard_owner_change(GtkClipboard
> > > > *clipboard,
> > > >  
> > > >      if (s->clip_grabbed[selection]) {
> > > >          s->clip_grabbed[selection] = FALSE;
> > > > -        if (spice_main_channel_agent_test_capability(s->main,
> > > > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> > > > -            spice_main_channel_clipboard_selection_release(s->main,
> > > > selection);
> > > > +        spice_main_channel_clipboard_selection_release(s->main,
> > > > selection);
> > > >      }
> > > >  
> > > >      switch (event->reason) {
> > > 
> > > Frediano

Thanks again :)