[Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure

[Victor Toso]

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 at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> > >
> > >
> > > --
> > > Marc-André Lureau
> 
> 
> 
> -- 
> Marc-André Lureau
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190116/070834c4/attachment.sig>