[Spice-devel,spice-gtk,v1] gtk-session: reply agent's clipboard request on failure

Submitted by Victor Toso on Feb. 22, 2017, 12:29 p.m.

Details

Message ID 20170222122920.22167-1-victortoso@redhat.com
State New
Headers show
Series "gtk-session: reply agent's clipboard request on failure" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Feb. 22, 2017, 12:29 p.m.
From: Victor Toso <me@victortoso.com>

We need to reply back to the agent all clipboard requests even in case
of failure otherwise it will have a pending request. The following
error message can be seen in afterwards, when client sends down some
clipboard data:

 > clipboard: selection requests pending on clipboard ownership
 > change, clearing

An easy way to reproduce this is:
1-) In client, copy image from lo-draw (selection or ctrl+c)
2-) In guest, paste it to GEdit (mouse3 our ctrl+v)
3-) Move to the client
4-) Move back to the guest
5-) see error on vdagent logs

The reason for failure is that client's clipboard contains different
data type (image) then what was requested from guest's editor (text)

While at it, include extra debug message as it might be hard to
identify why clipboard did not work.

Resolves: rhbz#1409854
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/spice-gtk-session.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 0426d8f..315bc26 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -940,28 +940,33 @@  static void clipboard_received_text_cb(GtkClipboard *clipboard,
     if (self == NULL)
         return;
 
+    selection = get_selection_from_clipboard(self->priv, clipboard);
+    g_return_if_fail(selection != -1);
+
     if (text == NULL) {
         SPICE_DEBUG("Failed to retrieve clipboard text");
-        return;
+        goto notify_agent;
     }
 
     g_return_if_fail(SPICE_IS_GTK_SESSION(self));
 
-    selection = get_selection_from_clipboard(self->priv, clipboard);
-    g_return_if_fail(selection != -1);
-
     len = strlen(text);
     if (!check_clipboard_size_limits(self, len)) {
-        return;
+        SPICE_DEBUG("Failed sized limits of clipboard text (%d bytes)", len);
+        len = 0;
+        goto notify_agent;
     }
 
     /* gtk+ internal utf8 newline is always LF, even on windows */
     conv = fixup_clipboard_text(self, text, &len);
     if (!check_clipboard_size_limits(self, len)) {
-        g_free(conv);
-        return;
+        SPICE_DEBUG("Failed sized limits of clipboard text (%d bytes)", len);
+        g_clear_pointer(&conv, g_free);
+        len = 0;
+        goto notify_agent;
     }
 
+notify_agent:
     spice_main_clipboard_selection_notify(self->priv->main, selection,
                                           VD_AGENT_CLIPBOARD_UTF8_TEXT,
                                           (guchar *)(conv ?: text), len);

Comments

Hi,

On Wed, Feb 22, 2017 at 01:29:20PM +0100, Victor Toso wrote:
> From: Victor Toso <me@victortoso.com>
>
> We need to reply back to the agent all clipboard requests even in case
> of failure otherwise it will have a pending request. The following
> error message can be seen in afterwards, when client sends down some
> clipboard data:

It is worth to mention in the commit log that this is a regression from
7b0de6217670e0f668aff2949f, as we were notifying the agent even on
failure, see:

https://gitlab.com/spice/spice-gtk/commit/7b0de6217670e0f668aff2949fba174ed3cc0b50#7d95131068c82b7224989c1c61cc3c85a9743df3_956_990

I've included in the commit log:

"This fixes a regression from 7b0de6217670e0f668aff2949f"

Cheers,

>
>  > clipboard: selection requests pending on clipboard ownership
>  > change, clearing
> 
> An easy way to reproduce this is:
> 1-) In client, copy image from lo-draw (selection or ctrl+c)
> 2-) In guest, paste it to GEdit (mouse3 our ctrl+v)
> 3-) Move to the client
> 4-) Move back to the guest
> 5-) see error on vdagent logs
> 
> The reason for failure is that client's clipboard contains different
> data type (image) then what was requested from guest's editor (text)
> 
> While at it, include extra debug message as it might be hard to
> identify why clipboard did not work.
> 
> Resolves: rhbz#1409854
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  src/spice-gtk-session.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 0426d8f..315bc26 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -940,28 +940,33 @@ static void clipboard_received_text_cb(GtkClipboard *clipboard,
>      if (self == NULL)
>          return;
>  
> +    selection = get_selection_from_clipboard(self->priv, clipboard);
> +    g_return_if_fail(selection != -1);
> +
>      if (text == NULL) {
>          SPICE_DEBUG("Failed to retrieve clipboard text");
> -        return;
> +        goto notify_agent;
>      }
>  
>      g_return_if_fail(SPICE_IS_GTK_SESSION(self));
>  
> -    selection = get_selection_from_clipboard(self->priv, clipboard);
> -    g_return_if_fail(selection != -1);
> -
>      len = strlen(text);
>      if (!check_clipboard_size_limits(self, len)) {
> -        return;
> +        SPICE_DEBUG("Failed sized limits of clipboard text (%d bytes)", len);
> +        len = 0;
> +        goto notify_agent;
>      }
>  
>      /* gtk+ internal utf8 newline is always LF, even on windows */
>      conv = fixup_clipboard_text(self, text, &len);
>      if (!check_clipboard_size_limits(self, len)) {
> -        g_free(conv);
> -        return;
> +        SPICE_DEBUG("Failed sized limits of clipboard text (%d bytes)", len);
> +        g_clear_pointer(&conv, g_free);
> +        len = 0;
> +        goto notify_agent;
>      }
>  
> +notify_agent:
>      spice_main_clipboard_selection_notify(self->priv->main, selection,
>                                            VD_AGENT_CLIPBOARD_UTF8_TEXT,
>                                            (guchar *)(conv ?: text), len);
> -- 
> 2.9.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On Fri, 2017-02-24 at 10:07 +0100, Victor Toso wrote:
> Hi,
> 
> On Wed, Feb 22, 2017 at 01:29:20PM +0100, Victor Toso wrote:
Maybe "gtk-session: Always reply to agent's clipboard request" fits
better

> > From: Victor Toso <me@victortoso.com>
> > 
> > We need to reply back to the agent all clipboard requests even in
> > case
> > of failure otherwise it will have a pending request. The following
> > error message can be seen in afterwards, when client sends down
> > some
> > clipboard data:
> 
> It is worth to mention in the commit log that this is a regression
> from
> 7b0de6217670e0f668aff2949f, as we were notifying the agent even on
> failure, see:
thanks
> 
> https://gitlab.com/spice/spice-gtk/commit/7b0de6217670e0f668aff2949f
> ba174ed3cc0b50#7d95131068c82b7224989c1c61cc3c85a9743df3_956_990
> 
> I've included in the commit log:
> 
> "This fixes a regression from 7b0de6217670e0f668aff2949f"
> 
> Cheers,
> 
> > 
> >  > clipboard: selection requests pending on clipboard ownership
> >  > change, clearing
> > 
> > An easy way to reproduce this is:
> > 1-) In client, copy image from lo-draw (selection or ctrl+c)
> > 2-) In guest, paste it to GEdit (mouse3 our ctrl+v)
> > 3-) Move to the client
> > 4-) Move back to the guest
> > 5-) see error on vdagent logs
> > 

This seems to be specific to the linux agent

> > The reason for failure is that client's clipboard contains
> > different
> > data type (image) then what was requested from guest's editor
> > (text)
> > 
> > While at it, include extra debug message as it might be hard to
> > identify why clipboard did not work.
> > 
> > Resolves: rhbz#1409854
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  src/spice-gtk-session.c | 19 ++++++++++++-------
> >  1 file changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 0426d8f..315bc26 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -940,28 +940,33 @@ static void
> > clipboard_received_text_cb(GtkClipboard *clipboard,
> >      if (self == NULL)
> >          return;
> >  
> > +    selection = get_selection_from_clipboard(self->priv,
> > clipboard);
> > +    g_return_if_fail(selection != -1);

so is it not needed to notify here as well ?

> > +
> >      if (text == NULL) {
> >          SPICE_DEBUG("Failed to retrieve clipboard text");
> > -        return;
> > +        goto notify_agent;
> >      }
> >  
> >      g_return_if_fail(SPICE_IS_GTK_SESSION(self));
> >  
> > -    selection = get_selection_from_clipboard(self->priv,
> > clipboard);
> > -    g_return_if_fail(selection != -1);
> > -
> >      len = strlen(text);
> >      if (!check_clipboard_size_limits(self, len)) {
> > -        return;
> > +        SPICE_DEBUG("Failed sized limits of clipboard text (%d
> > bytes)", len);
> > +        len = 0;
> > +        goto notify_agent;
> >      }
> >  
> >      /* gtk+ internal utf8 newline is always LF, even on windows
> > */
> >      conv = fixup_clipboard_text(self, text, &len);
> >      if (!check_clipboard_size_limits(self, len)) {
> > -        g_free(conv);
> > -        return;
> > +        SPICE_DEBUG("Failed sized limits of clipboard text (%d
> > bytes)", len);
> > +        g_clear_pointer(&conv, g_free);

thanks to goto there is no need to clear it

> > +        len = 0;
> > +        goto notify_agent;
> >      }
> >  
> > +notify_agent:
> >      spice_main_clipboard_selection_notify(self->priv->main,
> > selection,
> >                                            VD_AGENT_CLIPBOARD_UTF8
> > _TEXT,
> >                                            (guchar *)(conv ?:
> > text), len);
> > -- 
> > 2.9.3
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On Fri, Feb 24, 2017 at 10:25:43AM +0100, Pavel Grunt wrote:
> Hi,
>
> On Fri, 2017-02-24 at 10:07 +0100, Victor Toso wrote:
> > Hi,
> >
> > On Wed, Feb 22, 2017 at 01:29:20PM +0100, Victor Toso wrote:
> Maybe "gtk-session: Always reply to agent's clipboard request" fits
> better

Ok, fixed locally

>
> > > From: Victor Toso <me@victortoso.com>
> > > 
> > > We need to reply back to the agent all clipboard requests even in
> > > case
> > > of failure otherwise it will have a pending request. The following
> > > error message can be seen in afterwards, when client sends down
> > > some
> > > clipboard data:
> >
> > It is worth to mention in the commit log that this is a regression
> > from
> > 7b0de6217670e0f668aff2949f, as we were notifying the agent even on
> > failure, see:
> thanks
> >
> > https://gitlab.com/spice/spice-gtk/commit/7b0de6217670e0f668aff2949f
> > ba174ed3cc0b50#7d95131068c82b7224989c1c61cc3c85a9743df3_956_990
> > 
> > I've included in the commit log:
> > 
> > "This fixes a regression from 7b0de6217670e0f668aff2949f"
> > 
> > Cheers,
> > 
> > > 
> > >  > clipboard: selection requests pending on clipboard ownership
> > >  > change, clearing
> > > 
> > > An easy way to reproduce this is:
> > > 1-) In client, copy image from lo-draw (selection or ctrl+c)
> > > 2-) In guest, paste it to GEdit (mouse3 our ctrl+v)
> > > 3-) Move to the client
> > > 4-) Move back to the guest
> > > 5-) see error on vdagent logs
> > >
>
> This seems to be specific to the linux agent

Yes, we could improve the linux agent too if possible, in the future but
it makes sense to me that we reply an agent's request for clipboard
data even if it is empty.

>
> > > The reason for failure is that client's clipboard contains
> > > different
> > > data type (image) then what was requested from guest's editor
> > > (text)
> > >
> > > While at it, include extra debug message as it might be hard to
> > > identify why clipboard did not work.
> > >
> > > Resolves: rhbz#1409854
> > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > > ---
> > >  src/spice-gtk-session.c | 19 ++++++++++++-------
> > >  1 file changed, 12 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > index 0426d8f..315bc26 100644
> > > --- a/src/spice-gtk-session.c
> > > +++ b/src/spice-gtk-session.c
> > > @@ -940,28 +940,33 @@ static void
> > > clipboard_received_text_cb(GtkClipboard *clipboard,
> > >      if (self == NULL)
> > >          return;
> > >  
> > > +    selection = get_selection_from_clipboard(self->priv,
> > > clipboard);
> > > +    g_return_if_fail(selection != -1);
>
> so is it not needed to notify here as well ?

We can't without knowing the selection.

>
> > > +
> > >      if (text == NULL) {
> > >          SPICE_DEBUG("Failed to retrieve clipboard text");
> > > -        return;
> > > +        goto notify_agent;
> > >      }
> > >  
> > >      g_return_if_fail(SPICE_IS_GTK_SESSION(self));
> > >  
> > > -    selection = get_selection_from_clipboard(self->priv,
> > > clipboard);
> > > -    g_return_if_fail(selection != -1);
> > > -
> > >      len = strlen(text);
> > >      if (!check_clipboard_size_limits(self, len)) {
> > > -        return;
> > > +        SPICE_DEBUG("Failed sized limits of clipboard text (%d
> > > bytes)", len);
> > > +        len = 0;
> > > +        goto notify_agent;
> > >      }
> > >  
> > >      /* gtk+ internal utf8 newline is always LF, even on windows
> > > */
> > >      conv = fixup_clipboard_text(self, text, &len);
> > >      if (!check_clipboard_size_limits(self, len)) {
> > > -        g_free(conv);
> > > -        return;
> > > +        SPICE_DEBUG("Failed sized limits of clipboard text (%d
> > > bytes)", len);
> > > +        g_clear_pointer(&conv, g_free);
>
> thanks to goto there is no need to clear it

We do. Conv might not have failed, this check checks the
clipboard_size_limits so, if limit exceed the protocol limit, we need to
free conv here to send the right value to agent (nothing).

>
> > > +        len = 0;
> > > +        goto notify_agent;
> > >      }
> > >  
> > > +notify_agent:
> > >      spice_main_clipboard_selection_notify(self->priv->main,
> > > selection,
> > >                                            VD_AGENT_CLIPBOARD_UTF8
> > > _TEXT,
> > >                                            (guchar *)(conv ?:
> > > text), len);
> > > -- 
> > > 2.9.3
> > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Fri, 2017-02-24 at 10:49 +0100, Victor Toso wrote:
> Hi,
> 
> On Fri, Feb 24, 2017 at 10:25:43AM +0100, Pavel Grunt wrote:
> > Hi,
> > 
> > On Fri, 2017-02-24 at 10:07 +0100, Victor Toso wrote:
> > > Hi,
> > > 
> > > On Wed, Feb 22, 2017 at 01:29:20PM +0100, Victor Toso wrote:
> > 
> > Maybe "gtk-session: Always reply to agent's clipboard request"
> > fits
> > better
> 
> Ok, fixed locally
> 
> > 
> > > > From: Victor Toso <me@victortoso.com>
> > > > 
> > > > We need to reply back to the agent all clipboard requests even
> > > > in
> > > > case
> > > > of failure otherwise it will have a pending request. The
> > > > following
> > > > error message can be seen in afterwards, when client sends
> > > > down
> > > > some
> > > > clipboard data:
> > > 
> > > It is worth to mention in the commit log that this is a
> > > regression
> > > from
> > > 7b0de6217670e0f668aff2949f, as we were notifying the agent even
> > > on
> > > failure, see:
> > 
> > thanks
> > > 
> > > https://gitlab.com/spice/spice-gtk/commit/7b0de6217670e0f668aff2
> > > 949f
> > > ba174ed3cc0b50#7d95131068c82b7224989c1c61cc3c85a9743df3_956_990
> > > 
> > > I've included in the commit log:
> > > 
> > > "This fixes a regression from 7b0de6217670e0f668aff2949f"
> > > 
> > > Cheers,
> > > 
> > > > 
> > > >  > clipboard: selection requests pending on clipboard
> > > > ownership
> > > >  > change, clearing
> > > > 
> > > > An easy way to reproduce this is:
> > > > 1-) In client, copy image from lo-draw (selection or ctrl+c)
> > > > 2-) In guest, paste it to GEdit (mouse3 our ctrl+v)
> > > > 3-) Move to the client
> > > > 4-) Move back to the guest
> > > > 5-) see error on vdagent logs
> > > > 
> > 
> > This seems to be specific to the linux agent
> 
> Yes, we could improve the linux agent too if possible, in the future
> but
> it makes sense to me that we reply an agent's request for clipboard
> data even if it is empty.
> 
> > 
> > > > The reason for failure is that client's clipboard contains
> > > > different
> > > > data type (image) then what was requested from guest's editor
> > > > (text)
> > > > 
> > > > While at it, include extra debug message as it might be hard
> > > > to
> > > > identify why clipboard did not work.
> > > > 
> > > > Resolves: rhbz#1409854
> > > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > > > ---
> > > >  src/spice-gtk-session.c | 19 ++++++++++++-------
> > > >  1 file changed, 12 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > > index 0426d8f..315bc26 100644
> > > > --- a/src/spice-gtk-session.c
> > > > +++ b/src/spice-gtk-session.c
> > > > @@ -940,28 +940,33 @@ static void
> > > > clipboard_received_text_cb(GtkClipboard *clipboard,
> > > >      if (self == NULL)
> > > >          return;
> > > >  
> > > > +    selection = get_selection_from_clipboard(self->priv,
> > > > clipboard);
> > > > +    g_return_if_fail(selection != -1);
> > 
> > so is it not needed to notify here as well ?
> 
> We can't without knowing the selection.

hmhmhm, you can keep it, it is not related to the patch

the g_return is not needed, selection is used by a function in channel
main, and that function should verify it. There can be more
improvements (clipboard_received_text_cb is called from
clipboard_request which has info about the selection)

> 
> > 
> > > > +
> > > >      if (text == NULL) {
> > > >          SPICE_DEBUG("Failed to retrieve clipboard text");
> > > > -        return;
> > > > +        goto notify_agent;
> > > >      }
> > > >  
> > > >      g_return_if_fail(SPICE_IS_GTK_SESSION(self));
> > > >  
> > > > -    selection = get_selection_from_clipboard(self->priv,
> > > > clipboard);
> > > > -    g_return_if_fail(selection != -1);
> > > > -
> > > >      len = strlen(text);
> > > >      if (!check_clipboard_size_limits(self, len)) {
> > > > -        return;
> > > > +        SPICE_DEBUG("Failed sized limits of clipboard text
> > > > (%d
> > > > bytes)", len);
> > > > +        len = 0;
> > > > +        goto notify_agent;
> > > >      }
> > > >  
> > > >      /* gtk+ internal utf8 newline is always LF, even on
> > > > windows
> > > > */
> > > >      conv = fixup_clipboard_text(self, text, &len);
> > > >      if (!check_clipboard_size_limits(self, len)) {
> > > > -        g_free(conv);
> > > > -        return;
> > > > +        SPICE_DEBUG("Failed sized limits of clipboard text
> > > > (%d
> > > > bytes)", len);
> > > > +        g_clear_pointer(&conv, g_free);
> > 
> > thanks to goto there is no need to clear it
> 
> We do. Conv might not have failed, this check checks the
> clipboard_size_limits so, if limit exceed the protocol limit, we
> need to
> free conv here to send the right value to agent (nothing).

it sends len = 0


> 
> > 
> > > > +        len = 0;
> > > > +        goto notify_agent;
> > > >      }
> > > >  
> > > > +notify_agent:
> > > >      spice_main_clipboard_selection_notify(self->priv->main,
> > > > selection,
> > > >                                            VD_AGENT_CLIPBOARD_
> > > > UTF8
> > > > _TEXT,
> > > >                                            (guchar *)(conv ?:
> > > > text), len);
> > > > -- 
> > > > 2.9.3
> > > > 
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On Fri, Feb 24, 2017 at 11:04:38AM +0100, Pavel Grunt wrote:
> > > > > +    selection = get_selection_from_clipboard(self->priv,
> > > > > clipboard);
> > > > > +    g_return_if_fail(selection != -1);
> > >
> > > so is it not needed to notify here as well ?
> >
> > We can't without knowing the selection.
>
> hmhmhm, you can keep it, it is not related to the patch
>
> the g_return is not needed, selection is used by a function in channel
> main, and that function should verify it. There can be more
> improvements (clipboard_received_text_cb is called from
> clipboard_request which has info about the selection)

Okay, another improvement for later on them :)

> > > > >      conv = fixup_clipboard_text(self, text, &len);
> > > > >      if (!check_clipboard_size_limits(self, len)) {
> > > > > -        g_free(conv);
> > > > > -        return;
> > > > > +        SPICE_DEBUG("Failed sized limits of clipboard text
> > > > > (%d
> > > > > bytes)", len);
> > > > > +        g_clear_pointer(&conv, g_free);
> > >
> > > thanks to goto there is no need to clear it
> >
> > We do. Conv might not have failed, this check checks the
> > clipboard_size_limits so, if limit exceed the protocol limit, we
> > need to
> > free conv here to send the right value to agent (nothing).
>
> it sends len = 0

But I don't think it is right to put a valid buffer and 0 as len.
At least I think we should avoid it.

        toso
> On 22 Feb 2017, at 13:29, Victor Toso <victortoso@redhat.com> wrote:
> 
> From: Victor Toso <me@victortoso.com>
> 
> We need to reply back to the agent all clipboard requests even in case
> of failure otherwise it will have a pending request. The following
> error message can be seen in afterwards, when client sends down some
> clipboard data:
> 
>> clipboard: selection requests pending on clipboard ownership
>> change, clearing
> 
> An easy way to reproduce this is:
> 1-) In client, copy image from lo-draw (selection or ctrl+c)

The patch looks good to me, but one thing I don’t understand is why you modify a function called “clipboard_received_text_cb” when the description of how to reproduce the bug is when you send an image. Is that callback used even for non-text data? If so, could we also rename it?


> 2-) In guest, paste it to GEdit (mouse3 our ctrl+v)
> 3-) Move to the client
> 4-) Move back to the guest
> 5-) see error on vdagent logs

Is this the only place where we see the error? Would it be possible to have a non-intrusive notification that the paste operation did not work and why?

> 
> The reason for failure is that client's clipboard contains different
> data type (image) then what was requested from guest's editor (text)
> 
> While at it, include extra debug message as it might be hard to
> identify why clipboard did not work.

If it’s hard for you, imagine what it is for Joe MyGrandDadTheLinuxUser ;-)

> 
> Resolves: rhbz#1409854
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
> src/spice-gtk-session.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 0426d8f..315bc26 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -940,28 +940,33 @@ static void clipboard_received_text_cb(GtkClipboard *clipboard,
>    if (self == NULL)
>        return;
> 
> +    selection = get_selection_from_clipboard(self->priv, clipboard);
> +    g_return_if_fail(selection != -1);
> +
>    if (text == NULL) {
>        SPICE_DEBUG("Failed to retrieve clipboard text");
> -        return;
> +        goto notify_agent;
>    }
> 
>    g_return_if_fail(SPICE_IS_GTK_SESSION(self));
> 
> -    selection = get_selection_from_clipboard(self->priv, clipboard);
> -    g_return_if_fail(selection != -1);
> -
>    len = strlen(text);
>    if (!check_clipboard_size_limits(self, len)) {
> -        return;
> +        SPICE_DEBUG("Failed sized limits of clipboard text (%d bytes)", len);
> +        len = 0;
> +        goto notify_agent;
>    }
> 
>    /* gtk+ internal utf8 newline is always LF, even on windows */
>    conv = fixup_clipboard_text(self, text, &len);
>    if (!check_clipboard_size_limits(self, len)) {
> -        g_free(conv);
> -        return;
> +        SPICE_DEBUG("Failed sized limits of clipboard text (%d bytes)", len);
> +        g_clear_pointer(&conv, g_free);
> +        len = 0;
> +        goto notify_agent;
>    }
> 
> +notify_agent:
>    spice_main_clipboard_selection_notify(self->priv->main, selection,
>                                          VD_AGENT_CLIPBOARD_UTF8_TEXT,
>                                          (guchar *)(conv ?: text), len);
> -- 
> 2.9.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Fri, 2017-02-24 at 11:26 +0100, Victor Toso wrote:
> Hi,
> 
> On Fri, Feb 24, 2017 at 11:04:38AM +0100, Pavel Grunt wrote:
> > > > > > +    selection = get_selection_from_clipboard(self->priv,
> > > > > > clipboard);
> > > > > > +    g_return_if_fail(selection != -1);
> > > > 
> > > > so is it not needed to notify here as well ?
> > > 
> > > We can't without knowing the selection.
> > 
> > hmhmhm, you can keep it, it is not related to the patch
> > 
> > the g_return is not needed, selection is used by a function in
> > channel
> > main, and that function should verify it. There can be more
> > improvements (clipboard_received_text_cb is called from
> > clipboard_request which has info about the selection)
> 
> Okay, another improvement for later on them :)
> 
> > > > > >      conv = fixup_clipboard_text(self, text, &len);
> > > > > >      if (!check_clipboard_size_limits(self, len)) {
> > > > > > -        g_free(conv);
> > > > > > -        return;
> > > > > > +        SPICE_DEBUG("Failed sized limits of clipboard
> > > > > > text
> > > > > > (%d
> > > > > > bytes)", len);
> > > > > > +        g_clear_pointer(&conv, g_free);
> > > > 
> > > > thanks to goto there is no need to clear it
> > > 
> > > We do. Conv might not have failed, this check checks the
> > > clipboard_size_limits so, if limit exceed the protocol limit, we
> > > need to
> > > free conv here to send the right value to agent (nothing).
> > 
> > it sends len = 0
> 
> But I don't think it is right to put a valid buffer and 0 as len.

"text" is also nonnull - no difference to me

> At least I think we should avoid it.

> 
>         toso
Hi,

On Fri, Feb 24, 2017 at 12:36:56PM +0100, Pavel Grunt wrote:
> On Fri, 2017-02-24 at 11:26 +0100, Victor Toso wrote:
> > > > > > >      conv = fixup_clipboard_text(self, text, &len);
> > > > > > >      if (!check_clipboard_size_limits(self, len)) {
> > > > > > > -        g_free(conv);
> > > > > > > -        return;
> > > > > > > +        SPICE_DEBUG("Failed sized limits of clipboard
> > > > > > > text
> > > > > > > (%d
> > > > > > > bytes)", len);
> > > > > > > +        g_clear_pointer(&conv, g_free);
> > > > >
> > > > > thanks to goto there is no need to clear it
> > > >
> > > > We do. Conv might not have failed, this check checks the
> > > > clipboard_size_limits so, if limit exceed the protocol limit, we
> > > > need to
> > > > free conv here to send the right value to agent (nothing).
> > >
> > > it sends len = 0
> >
> > But I don't think it is right to put a valid buffer and 0 as len.
>
> "text" is also nonnull - no difference to me

I missed it. Still, no good :(
The chain up calls will be:

1-) spice_main_clipboard_selection_notify() (with data != NULL)
2-) agent_clipboard_notify()
3-) agent_msg_queue_many()
    -> and here we do:

980     va_start(args, data);
981     for (d = data; d != NULL; d = va_arg(args, void*)) {
982         size += va_arg(args, gsize);
983     }
984     va_end(args);

    -> which changes the size of the message if data is nonnull.

I'll fix it. Nice catch!!!

        toso
Hi,

On Fri, Feb 24, 2017 at 12:13:21PM +0100, Christophe de Dinechin wrote:
>
> > On 22 Feb 2017, at 13:29, Victor Toso <victortoso@redhat.com> wrote:
> >
> > From: Victor Toso <me@victortoso.com>
> >
> > We need to reply back to the agent all clipboard requests even in case
> > of failure otherwise it will have a pending request. The following
> > error message can be seen in afterwards, when client sends down some
> > clipboard data:
> >
> >> clipboard: selection requests pending on clipboard ownership
> >> change, clearing
> >
> > An easy way to reproduce this is:
> > 1-) In client, copy image from lo-draw (selection or ctrl+c)
>
> The patch looks good to me, but one thing I don’t understand is why
> you modify a function called “clipboard_received_text_cb” when the
> description of how to reproduce the bug is when you send an image. Is
> that callback used even for non-text data? If so, could we also rename
> it?

The problem is that the clipboard data that was requested is of TEXT
type. That will fail because we have an image in the clipboard.

These were the easiest way to reproduce I've found also aligned with the
https://bugzilla.redhat.com/show_bug.cgi?id=1409854#c0

> > 2-) In guest, paste it to GEdit (mouse3 our ctrl+v)
> > 3-) Move to the client
> > 4-) Move back to the guest
> > 5-) see error on vdagent logs
>
> Is this the only place where we see the error? Would it be possible to
> have a non-intrusive notification that the paste operation did not
> work and why?

We don't have this error on the spice-gtk nowadays. I think it would be
a good RFE but no clue on how to make it non-intrusive in the clients.

This is happening a lot with wayland even between two applications in
the client machine (e.g copying a url in gnome-terminal and pasting it
in the epiphany browser) - I don't see how it can inform user that
clipboard is failing there too :(

> > The reason for failure is that client's clipboard contains different
> > data type (image) then what was requested from guest's editor (text)
> >
> > While at it, include extra debug message as it might be hard to
> > identify why clipboard did not work.
>
> If it’s hard for you, imagine what it is for Joe
> MyGrandDadTheLinuxUser ;-)

:-)

>
> >
> > Resolves: rhbz#1409854
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> > src/spice-gtk-session.c | 19 ++++++++++++-------
> > 1 file changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 0426d8f..315bc26 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -940,28 +940,33 @@ static void clipboard_received_text_cb(GtkClipboard *clipboard,
> >    if (self == NULL)
> >        return;
> > 
> > +    selection = get_selection_from_clipboard(self->priv, clipboard);
> > +    g_return_if_fail(selection != -1);
> > +
> >    if (text == NULL) {
> >        SPICE_DEBUG("Failed to retrieve clipboard text");
> > -        return;
> > +        goto notify_agent;
> >    }
> > 
> >    g_return_if_fail(SPICE_IS_GTK_SESSION(self));
> > 
> > -    selection = get_selection_from_clipboard(self->priv, clipboard);
> > -    g_return_if_fail(selection != -1);
> > -
> >    len = strlen(text);
> >    if (!check_clipboard_size_limits(self, len)) {
> > -        return;
> > +        SPICE_DEBUG("Failed sized limits of clipboard text (%d bytes)", len);
> > +        len = 0;
> > +        goto notify_agent;
> >    }
> > 
> >    /* gtk+ internal utf8 newline is always LF, even on windows */
> >    conv = fixup_clipboard_text(self, text, &len);
> >    if (!check_clipboard_size_limits(self, len)) {
> > -        g_free(conv);
> > -        return;
> > +        SPICE_DEBUG("Failed sized limits of clipboard text (%d bytes)", len);
> > +        g_clear_pointer(&conv, g_free);
> > +        len = 0;
> > +        goto notify_agent;
> >    }
> > 
> > +notify_agent:
> >    spice_main_clipboard_selection_notify(self->priv->main, selection,
> >                                          VD_AGENT_CLIPBOARD_UTF8_TEXT,
> >                                          (guchar *)(conv ?: text), len);
> > -- 
> > 2.9.3
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>