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

[Marc-André Lureau]

Hi

On Wed, Jan 16, 2019 at 1:18 PM Victor Toso <victortoso at redhat.com> 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 at lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > >
> > > >
> > > >
> > > > --
> > > > Marc-André Lureau
> >
> >
> >
> > --
> > Marc-André Lureau



-- 
Marc-André Lureau