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 <victortoso@xxxxxxxxxx> 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;
> > > > > > +
> > > > > > +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@xxxxxxxxxxxxxxxxxxxxx
> > > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Marc-André Lureau
> > >
> > >
> > >
> > > --
> > > Marc-André Lureau
>
>
>
> --
> Marc-André Lureau
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel