Re: [Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure
Hi,
On Wed, Jan 16, 2019 at 02:33:49PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Wed, Jan 16, 2019 at 1:18 PM Victor Toso wrote:
> >
> > Hi,
> >
> > On Wed, Jan 16, 2019 at 01:07:27PM +0400, Marc-André Lureau wrote:
> > > > Not true. Possible client-side bug, reproducible on X11, as I
> > > > discussed before. As mentioned in the comment, we are sending
> > > > wrong selection-grab to the guest.
> > >
> > > Sorry, I don't follow "we are sending wrong selection-grab" ?
> >
> > I think I tried too hard already to make you understand that on
> > X11, that's happening while user is copying in the guest VM.
>
> Sorry, I don't know what you are talking about. When there is a
> complicated question/reply, you should update the comments or
> commit message, so we don't have to dig in old conversations.
Old... Yesterday and the day before and last week...
https://lists.freedesktop.org/archives/spice-devel/2019-January/047192.html
Commit log:
A way to reproduce is copy-paste big clipboard data between
two spice clients.
It is missing 'on x11'.
> > > > > Not sure sending a reply is going to help much in that case...
> > > >
> > > > It helps. Instead of blocking the application, it fails to paste
> > > > the clipboard and all is good.
> > >
> > > But if the agent logic is already wrong, sending a reply isn't going
> > > to help, it could do anything..
> >
> > It might have been a valid request at the time it was sent, if
> > you accept that we sent the selection-grab.
>
> "we" == the client?
>
>
> In this case, clipboard_by_guest[] should be false.
I give up Feel free to ignore.
>
> >
> > > > > > + * sent a bad selection-grab to the agent or the agent is
> > > > > > buggy. */
> > > > > > +if (s->clipboard_by_guest[selection]) {
> > > > > > +SPICE_DEBUG("clipboard: agent request: grab on hold by
> > > > > > agent, possible race");
> > > > > > +goto notify_agent;
> > > > > > +}
> > > > > > +
> > > > > > +/* The selection-request by agent should happen only if the
> > > > > > clipboard data is set
> > > > > > + * by client */
> > > > > > +if (!s->clip_grabbed[selection]) {
> > > > > > +SPICE_DEBUG("clipboard: agent request: data set by agent,
> > > > > > possible race");
> > > > > > +goto notify_agent;
> > > > > > +}
> > > > >
> > > > > This could be adding more race conditions. clip_grabbed is set
> > > > > asynchronously after owner changed, and indicate if the grab
> > > > > message was sent to the agent,
> > > >
> > > > We send a selection-release on owner-change. If we receive a
> > > > selection-request before agent receives the selection-release,
> > > > this is expected but we should notify the guest, afaics.
> > > >
> > > > > as you correctly say. It doesn't mean you can't request client
> > > > > clipboard content.
> > > >
> > > > > I understand the racy case, but the condition seems wrong, it
> > > > > should attempt to request current client clipboard content, and
> > > > > fail/succeed after.
> > > >
> > > > No. It should request the content from the grab that was sent. If
> > >
> > > There is no such thing as clipboard ID/nth in any clipboard protocol I
> > > know of. User simply request current clipboard content (not the
> > > nth), whether it changed since last grab is not important: it
> > > will retrieve something of the requested type or nothing.
> >
> > Not about the id, but selection metadata itself or the @type
> > parameter in clipboard_request()
>
> If the clipboard content (of the requested type) can't be retrieve, it
> will fail. It shouldn't be handled here.
>
> >
> > > > clipboard changed, previous grab gets invalidated and a
> > > > selection-release is sent and another selection-grab is sent with
> > > > the metadata of the new grab.
> > > >
> > > > Proving recent data from old grab seems wrong.
> > >
> > > It is not.
> > >
> > > > >
> > > > > > +/* Ready only, still should reply to agent to avoid it waiting
> > > > > > for data */
> > > > >
> > > > > No, read-only shouldn't reply. We are lacking a way to tell the guest
> > > > > that the client is read-only though. So this may be acceptable for
> > > > > now, but we should have a TODO/FIXME..
> > > >
> > > > Well, it should not happen anyway as on read-only we should not
> > > > send selection-grab. I'll fix and resend.
> > >
> > > Right
> > >
> > > >
> > > > Thanks for the quick review.
> > > >
> > > > > > +if (read_only(self)) {
> > > > > > +g_warning("clipboard: agent request: read only, deny
> > > > > > request");
> > > > > > +goto notify_agent;
> > > > > > +}
> > > > > >
> > > > > > if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
> > > > > > gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> > > > > > @@ -1039,6 +1057,10 @@ static gboolean
> > > > > > clipboard_request(SpiceMainChannel *main, guint selection,
> > > > > > }
> > > > > >
> > > > > > return TRUE;
> > > > > > +
> >
Re: [Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure
Hi
On Wed, Jan 16, 2019 at 1:18 PM Victor Toso wrote:
>
> Hi,
>
> On Wed, Jan 16, 2019 at 01:07:27PM +0400, Marc-André Lureau wrote:
> > > Not true. Possible client-side bug, reproducible on X11, as I
> > > discussed before. As mentioned in the comment, we are sending
> > > wrong selection-grab to the guest.
> >
> > Sorry, I don't follow "we are sending wrong selection-grab" ?
>
> I think I tried too hard already to make you understand that on
> X11, that's happening while user is copying in the guest VM.
Sorry, I don't know what you are talking about. When there is a
complicated question/reply, you should update the comments or commit
message, so we don't have to dig in old conversations.
>
> > > > Not sure sending a reply is going to help much in that case...
> > >
> > > It helps. Instead of blocking the application, it fails to paste
> > > the clipboard and all is good.
> >
> > But if the agent logic is already wrong, sending a reply isn't going
> > to help, it could do anything..
>
> It might have been a valid request at the time it was sent, if
> you accept that we sent the selection-grab.
"we" == the client?
In this case, clipboard_by_guest[] should be false.
>
> > > > > + * sent a bad selection-grab to the agent or the agent is buggy.
> > > > > */
> > > > > +if (s->clipboard_by_guest[selection]) {
> > > > > +SPICE_DEBUG("clipboard: agent request: grab on hold by
> > > > > agent, possible race");
> > > > > +goto notify_agent;
> > > > > +}
> > > > > +
> > > > > +/* The selection-request by agent should happen only if the
> > > > > clipboard data is set
> > > > > + * by client */
> > > > > +if (!s->clip_grabbed[selection]) {
> > > > > +SPICE_DEBUG("clipboard: agent request: data set by agent,
> > > > > possible race");
> > > > > +goto notify_agent;
> > > > > +}
> > > >
> > > > This could be adding more race conditions. clip_grabbed is set
> > > > asynchronously after owner changed, and indicate if the grab
> > > > message was sent to the agent,
> > >
> > > We send a selection-release on owner-change. If we receive a
> > > selection-request before agent receives the selection-release,
> > > this is expected but we should notify the guest, afaics.
> > >
> > > > as you correctly say. It doesn't mean you can't request client
> > > > clipboard content.
> > >
> > > > I understand the racy case, but the condition seems wrong, it
> > > > should attempt to request current client clipboard content, and
> > > > fail/succeed after.
> > >
> > > No. It should request the content from the grab that was sent. If
> >
> > There is no such thing as clipboard ID/nth in any clipboard protocol I
> > know of. User simply request current clipboard content (not the
> > nth), whether it changed since last grab is not important: it
> > will retrieve something of the requested type or nothing.
>
> Not about the id, but selection metadata itself or the @type
> parameter in clipboard_request()
If the clipboard content (of the requested type) can't be retrieve, it
will fail. It shouldn't be handled here.
>
> > > clipboard changed, previous grab gets invalidated and a
> > > selection-release is sent and another selection-grab is sent with
> > > the metadata of the new grab.
> > >
> > > Proving recent data from old grab seems wrong.
> >
> > It is not.
> >
> > > >
> > > > > +/* Ready only, still should reply to agent to avoid it waiting
> > > > > for data */
> > > >
> > > > No, read-only shouldn't reply. We are lacking a way to tell the guest
> > > > that the client is read-only though. So this may be acceptable for
> > > > now, but we should have a TODO/FIXME..
> > >
> > > Well, it should not happen anyway as on read-only we should not
> > > send selection-grab. I'll fix and resend.
> >
> > Right
> >
> > >
> > > Thanks for the quick review.
> > >
> > > > > +if (read_only(self)) {
> > > > > +g_warning("clipboard: agent request: read only, deny
> > > > > request");
> > > > > +goto notify_agent;
> > > > > +}
> > > > >
> > > > > if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
> > > > > gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> > > > > @@ -1039,6 +1057,10 @@ static gboolean
> > > > > clipboard_request(SpiceMainChannel *main, guint selection,
> > > > > }
> > > > >
> > > > > return TRUE;
> > > > > +
> > > > > +notify_agent:
> > > > > +spice_main_channel_clipboard_selection_notify(s->main,
> > > > > selection, type, NULL, 0);
> > > > > +return FALSE;
> > > > > }
> > > > >
> > > > > static void clipboard_release(SpiceMainChannel *main, guint
> > > > > selection,
> > > > > --
> > > > > 2.20.1
> > > > >
> > > > > ___
> > > > > Spice-devel mailing list
> > > > > Spice-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > >
> > > >
> > > >
> > > > --
> > > > Marc-André Lureau
> >
> >
> >
> > --
> >
Re: [Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure
Hi,
On Wed, Jan 16, 2019 at 01:07:27PM +0400, Marc-André Lureau wrote:
> > Not true. Possible client-side bug, reproducible on X11, as I
> > discussed before. As mentioned in the comment, we are sending
> > wrong selection-grab to the guest.
>
> Sorry, I don't follow "we are sending wrong selection-grab" ?
I think I tried too hard already to make you understand that on
X11, that's happening while user is copying in the guest VM.
> > > Not sure sending a reply is going to help much in that case...
> >
> > It helps. Instead of blocking the application, it fails to paste
> > the clipboard and all is good.
>
> But if the agent logic is already wrong, sending a reply isn't going
> to help, it could do anything..
It might have been a valid request at the time it was sent, if
you accept that we sent the selection-grab.
> > > > + * sent a bad selection-grab to the agent or the agent is buggy. */
> > > > +if (s->clipboard_by_guest[selection]) {
> > > > +SPICE_DEBUG("clipboard: agent request: grab on hold by agent,
> > > > possible race");
> > > > +goto notify_agent;
> > > > +}
> > > > +
> > > > +/* The selection-request by agent should happen only if the
> > > > clipboard data is set
> > > > + * by client */
> > > > +if (!s->clip_grabbed[selection]) {
> > > > +SPICE_DEBUG("clipboard: agent request: data set by agent,
> > > > possible race");
> > > > +goto notify_agent;
> > > > +}
> > >
> > > This could be adding more race conditions. clip_grabbed is set
> > > asynchronously after owner changed, and indicate if the grab
> > > message was sent to the agent,
> >
> > We send a selection-release on owner-change. If we receive a
> > selection-request before agent receives the selection-release,
> > this is expected but we should notify the guest, afaics.
> >
> > > as you correctly say. It doesn't mean you can't request client
> > > clipboard content.
> >
> > > I understand the racy case, but the condition seems wrong, it
> > > should attempt to request current client clipboard content, and
> > > fail/succeed after.
> >
> > No. It should request the content from the grab that was sent. If
>
> There is no such thing as clipboard ID/nth in any clipboard protocol I
> know of. User simply request current clipboard content (not the
> nth), whether it changed since last grab is not important: it
> will retrieve something of the requested type or nothing.
Not about the id, but selection metadata itself or the @type
parameter in clipboard_request()
> > clipboard changed, previous grab gets invalidated and a
> > selection-release is sent and another selection-grab is sent with
> > the metadata of the new grab.
> >
> > Proving recent data from old grab seems wrong.
>
> It is not.
>
> > >
> > > > +/* Ready only, still should reply to agent to avoid it waiting for
> > > > data */
> > >
> > > No, read-only shouldn't reply. We are lacking a way to tell the guest
> > > that the client is read-only though. So this may be acceptable for
> > > now, but we should have a TODO/FIXME..
> >
> > Well, it should not happen anyway as on read-only we should not
> > send selection-grab. I'll fix and resend.
>
> Right
>
> >
> > Thanks for the quick review.
> >
> > > > +if (read_only(self)) {
> > > > +g_warning("clipboard: agent request: read only, deny request");
> > > > +goto notify_agent;
> > > > +}
> > > >
> > > > if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
> > > > gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> > > > @@ -1039,6 +1057,10 @@ static gboolean
> > > > clipboard_request(SpiceMainChannel *main, guint selection,
> > > > }
> > > >
> > > > return TRUE;
> > > > +
> > > > +notify_agent:
> > > > +spice_main_channel_clipboard_selection_notify(s->main, selection,
> > > > type, NULL, 0);
> > > > +return FALSE;
> > > > }
> > > >
> > > > static void clipboard_release(SpiceMainChannel *main, guint selection,
> > > > --
> > > > 2.20.1
> > > >
> > > > ___
> > > > Spice-devel mailing list
> > > > Spice-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> > >
> > >
> > > --
> > > Marc-André Lureau
>
>
>
> --
> Marc-André Lureau
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure
Hi
On Wed, Jan 16, 2019 at 12:56 PM Victor Toso wrote:
>
> Hi,
>
> On Wed, Jan 16, 2019 at 12:40:36PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Jan 16, 2019 at 11:44 AM Victor Toso wrote:
> > >
> > > From: Victor Toso
> > >
> > > Similar to 172c521271a3d - if we don't, the agent might be waiting for
> > > data till some timeout happens in the system, blocking copy-paste
> > > feature and possibly freezing applications.
> > >
> > > A way to reproduce is copy-paste big clipboard data between two spice
> > > clients.
> > >
> > > Also add some documentation to the checks, in order to quickly
> > > understand what they are about.
> > >
> > > Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> > > Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> > >
> > > Signed-off-by: Victor Toso
> > > ---
> > > src/spice-gtk-session.c | 32 +++-
> > > 1 file changed, 27 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > index 1a19bca..aa812d1 100644
> > > --- a/src/spice-gtk-session.c
> > > +++ b/src/spice-gtk-session.c
> > > @@ -1015,12 +1015,30 @@ static gboolean
> > > clipboard_request(SpiceMainChannel *main, guint selection,
> > > int m;
> > >
> > > cb = get_clipboard_from_selection(s, selection);
> > > -g_return_val_if_fail(cb != NULL, FALSE);
> > > -g_return_val_if_fail(s->clipboard_by_guest[selection] == FALSE,
> > > FALSE);
> > > -g_return_val_if_fail(s->clip_grabbed[selection], FALSE);
> > > +if (cb == NULL) {
> > > +goto notify_agent;
> > > +}
> > >
> > > -if (read_only(self))
> > > -return FALSE;
> > > +/* As we are holding clipboard data sent by selection-grab from
> > > agent, the
> > > + * selection-request of clipboard data from client OS must fail. We
> > > either
> >
> > from client OS? Here it's a signal handler for guest selection-request.
>
> Yes, comment is wrong, should be:
>
> /* As we are holding clipboard data sent by selection-grab from guest, the
> * selection-request of clipboard data from guest must fail. We either
> * sent a bad selection-grab to the guest or the agent is buggy. */
>
> > This would clearly be a guest-side bug if we reach that
> > condition (events are processed in order, and
> > clipboard_by_guest is set synchronously).
>
> Not true. Possible client-side bug, reproducible on X11, as I
> discussed before. As mentioned in the comment, we are sending
> wrong selection-grab to the guest.
Sorry, I don't follow "we are sending wrong selection-grab" ?
>
> > Not sure sending a reply is going to help much in that case...
>
> It helps. Instead of blocking the application, it fails to paste
> the clipboard and all is good.
But if the agent logic is already wrong, sending a reply isn't going
to help, it could do anything..
>
> > > + * sent a bad selection-grab to the agent or the agent is buggy. */
> > > +if (s->clipboard_by_guest[selection]) {
> > > +SPICE_DEBUG("clipboard: agent request: grab on hold by agent,
> > > possible race");
> > > +goto notify_agent;
> > > +}
> > > +
> > > +/* The selection-request by agent should happen only if the
> > > clipboard data is set
> > > + * by client */
> > > +if (!s->clip_grabbed[selection]) {
> > > +SPICE_DEBUG("clipboard: agent request: data set by agent,
> > > possible race");
> > > +goto notify_agent;
> > > +}
> >
> > This could be adding more race conditions. clip_grabbed is set
> > asynchronously after owner changed, and indicate if the grab
> > message was sent to the agent,
>
> We send a selection-release on owner-change. If we receive a
> selection-request before agent receives the selection-release,
> this is expected but we should notify the guest, afaics.
>
> > as you correctly say. It doesn't mean you can't request client
> > clipboard content.
>
> > I understand the racy case, but the condition seems wrong, it
> > should attempt to request current client clipboard content, and
> > fail/succeed after.
>
> No. It should request the content from the grab that was sent. If
There is no such thing as clipboard ID/nth in any clipboard protocol I
know of. User simply request current clipboard content (not the nth),
whether it changed since last grab is not important: it will retrieve
something of the requested type or nothing.
> clipboard changed, previous grab gets invalidated and a
> selection-release is sent and another selection-grab is sent with
> the metadata of the new grab.
>
> Proving recent data from old grab seems wrong.
It is not.
> >
> > > +/* Ready only, still should reply to agent to avoid it waiting for
> > > data */
> >
> > No, read-only shouldn't reply. We are lacking a way to tell the guest
> > that the client is read-only though. So this may be acceptable for
> >
Re: [Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure
Hi,
On Wed, Jan 16, 2019 at 12:40:36PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Wed, Jan 16, 2019 at 11:44 AM Victor Toso wrote:
> >
> > From: Victor Toso
> >
> > Similar to 172c521271a3d - if we don't, the agent might be waiting for
> > data till some timeout happens in the system, blocking copy-paste
> > feature and possibly freezing applications.
> >
> > A way to reproduce is copy-paste big clipboard data between two spice
> > clients.
> >
> > Also add some documentation to the checks, in order to quickly
> > understand what they are about.
> >
> > Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> > Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> >
> > Signed-off-by: Victor Toso
> > ---
> > src/spice-gtk-session.c | 32 +++-
> > 1 file changed, 27 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 1a19bca..aa812d1 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -1015,12 +1015,30 @@ static gboolean clipboard_request(SpiceMainChannel
> > *main, guint selection,
> > int m;
> >
> > cb = get_clipboard_from_selection(s, selection);
> > -g_return_val_if_fail(cb != NULL, FALSE);
> > -g_return_val_if_fail(s->clipboard_by_guest[selection] == FALSE, FALSE);
> > -g_return_val_if_fail(s->clip_grabbed[selection], FALSE);
> > +if (cb == NULL) {
> > +goto notify_agent;
> > +}
> >
> > -if (read_only(self))
> > -return FALSE;
> > +/* As we are holding clipboard data sent by selection-grab from agent,
> > the
> > + * selection-request of clipboard data from client OS must fail. We
> > either
>
> from client OS? Here it's a signal handler for guest selection-request.
Yes, comment is wrong, should be:
/* As we are holding clipboard data sent by selection-grab from guest, the
* selection-request of clipboard data from guest must fail. We either
* sent a bad selection-grab to the guest or the agent is buggy. */
> This would clearly be a guest-side bug if we reach that
> condition (events are processed in order, and
> clipboard_by_guest is set synchronously).
Not true. Possible client-side bug, reproducible on X11, as I
discussed before. As mentioned in the comment, we are sending
wrong selection-grab to the guest.
> Not sure sending a reply is going to help much in that case...
It helps. Instead of blocking the application, it fails to paste
the clipboard and all is good.
> > + * sent a bad selection-grab to the agent or the agent is buggy. */
> > +if (s->clipboard_by_guest[selection]) {
> > +SPICE_DEBUG("clipboard: agent request: grab on hold by agent,
> > possible race");
> > +goto notify_agent;
> > +}
> > +
> > +/* The selection-request by agent should happen only if the clipboard
> > data is set
> > + * by client */
> > +if (!s->clip_grabbed[selection]) {
> > +SPICE_DEBUG("clipboard: agent request: data set by agent, possible
> > race");
> > +goto notify_agent;
> > +}
>
> This could be adding more race conditions. clip_grabbed is set
> asynchronously after owner changed, and indicate if the grab
> message was sent to the agent,
We send a selection-release on owner-change. If we receive a
selection-request before agent receives the selection-release,
this is expected but we should notify the guest, afaics.
> as you correctly say. It doesn't mean you can't request client
> clipboard content.
> I understand the racy case, but the condition seems wrong, it
> should attempt to request current client clipboard content, and
> fail/succeed after.
No. It should request the content from the grab that was sent. If
clipboard changed, previous grab gets invalidated and a
selection-release is sent and another selection-grab is sent with
the metadata of the new grab.
Proving recent data from old grab seems wrong.
>
> > +/* Ready only, still should reply to agent to avoid it waiting for
> > data */
>
> No, read-only shouldn't reply. We are lacking a way to tell the guest
> that the client is read-only though. So this may be acceptable for
> now, but we should have a TODO/FIXME..
Well, it should not happen anyway as on read-only we should not
send selection-grab. I'll fix and resend.
Thanks for the quick review.
> > +if (read_only(self)) {
> > +g_warning("clipboard: agent request: read only, deny request");
> > +goto notify_agent;
> > +}
> >
> > if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
> > gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> > @@ -1039,6 +1057,10 @@ static gboolean clipboard_request(SpiceMainChannel
> > *main, guint selection,
> > }
> >
> > return TRUE;
> > +
> > +notify_agent:
> > +spice_main_channel_clipboard_selection_notify(s->main, selection,
> > type,
Re: [Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure
Hi
On Wed, Jan 16, 2019 at 11:44 AM Victor Toso wrote:
>
> From: Victor Toso
>
> Similar to 172c521271a3d - if we don't, the agent might be waiting for
> data till some timeout happens in the system, blocking copy-paste
> feature and possibly freezing applications.
>
> A way to reproduce is copy-paste big clipboard data between two spice
> clients.
>
> Also add some documentation to the checks, in order to quickly
> understand what they are about.
>
> Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
>
> Signed-off-by: Victor Toso
> ---
> src/spice-gtk-session.c | 32 +++-
> 1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 1a19bca..aa812d1 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -1015,12 +1015,30 @@ static gboolean clipboard_request(SpiceMainChannel
> *main, guint selection,
> int m;
>
> cb = get_clipboard_from_selection(s, selection);
> -g_return_val_if_fail(cb != NULL, FALSE);
> -g_return_val_if_fail(s->clipboard_by_guest[selection] == FALSE, FALSE);
> -g_return_val_if_fail(s->clip_grabbed[selection], FALSE);
> +if (cb == NULL) {
> +goto notify_agent;
> +}
>
> -if (read_only(self))
> -return FALSE;
> +/* As we are holding clipboard data sent by selection-grab from agent,
> the
> + * selection-request of clipboard data from client OS must fail. We
> either
from client OS? Here it's a signal handler for guest selection-request.
This would clearly be a guest-side bug if we reach that condition
(events are processed in order, and clipboard_by_guest is set
synchronously). Not sure sending a reply is going to help much in that
case...
> + * sent a bad selection-grab to the agent or the agent is buggy. */
> +if (s->clipboard_by_guest[selection]) {
> +SPICE_DEBUG("clipboard: agent request: grab on hold by agent,
> possible race");
> +goto notify_agent;
> +}
> +
> +/* The selection-request by agent should happen only if the clipboard
> data is set
> + * by client */
> +if (!s->clip_grabbed[selection]) {
> +SPICE_DEBUG("clipboard: agent request: data set by agent, possible
> race");
> +goto notify_agent;
> +}
This could be adding more race conditions. clip_grabbed is set
asynchronously after owner changed, and indicate if the grab message
was sent to the agent, as you correctly say. It doesn't mean you can't
request client clipboard content.
I understand the racy case, but the condition seems wrong, it should
attempt to request current client clipboard content, and fail/succeed
after.
> +/* Ready only, still should reply to agent to avoid it waiting for data
> */
No, read-only shouldn't reply. We are lacking a way to tell the guest
that the client is read-only though. So this may be acceptable for
now, but we should have a TODO/FIXME..
> +if (read_only(self)) {
> +g_warning("clipboard: agent request: read only, deny request");
> +goto notify_agent;
> +}
>
> if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
> gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> @@ -1039,6 +1057,10 @@ static gboolean clipboard_request(SpiceMainChannel
> *main, guint selection,
> }
>
> return TRUE;
> +
> +notify_agent:
> +spice_main_channel_clipboard_selection_notify(s->main, selection, type,
> NULL, 0);
> +return FALSE;
> }
>
> static void clipboard_release(SpiceMainChannel *main, guint selection,
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
