[Spice-devel] [PATCH spice-protocol 3/3] vdagent: introduce VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL

[Marc-André Lureau]

Hi

On Wed, Mar 27, 2019 at 4:50 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> > Hi
> >
> > On Wed, Mar 27, 2019 at 8:23 AM Frediano Ziglio <fziglio at redhat.com> wrote:
> > >
> > > >
> > > > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > >
> > > > When this capability is negoticated by both the client & the agent,
> > >
> > > negotiated
> > >
> > > > the clipboard grab messages have an associated serial counter.
> > > >
> > > > The serial is reset to 0 upon client connection.
> > > >
> > > > The counter is increment by 1 on each grab message, by both sides.
> > > >
> > >
> > > What would happen in case of restart of the guest? How the guest is
> > > supposed to keep the old serial?
> >
> > This is like a new client-agent connection in this case, it starts again from
> > 0.
> >
> > > I think you can achieve sending the serial with an additional separate
> > > message at the beginning.
> >
> > I don't think it's necessary, but I am may be missing something.
> >
>
> Well, adding a field or a message are both changes, just different.
>
> > > I don't think this new protocol is designed to support multiple
> > > clients.
> >
> > clipboard sharing isn't designed for multiple client either.
> >
> > >
> > > > The sender of the message with the highest serial should be the
> > > > clipboard grab owner, and the current session serial should be
> > > > updated.
> > > >
> > > > If a lower serial than the current session serial is received, the
> > > > grab should be discarded.
> > > >
> > > > Whenever two grabs share the same serial, the one coming from the
> > > > client should have a higher priority and the client should gain the
> > > > clipboard ownership.
> > > >
> > > > No special treatement is done for the unlikely case of overflowing the
> > >
> > > treatment
> >
> > ok
> >
> > >
> > > > counter. It may temporarily inverse the priority, until both side have
> > > > overflown and/or synchronized.
> > > >
> > >
> > > synchronized? So there's a single counter for both guest and client?
> > > I though were two counters, one for each side.
> >
> > Conceptually, it's the "same" counter.
> >
>
> Okay, it was not clear.
>
> > >
> > > > Note: this mechanism isn't aiming at making "the most recent" (as in
> > > > time) side gaining the ownership. One side sending subsequent grab
> > > > messages earlier will likely take the ownership over a side sending a
> > > > single message simultaneously the other way. It only clears the
> > > > situation where both side believe that the other is the current
> > > > clipboard owner, by having a global ordering and priority in case of
> > > > serial conflict.
> > > >
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > > ---
> > > >  spice/vd_agent.h | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > > > index 862cb5c..9ae3cc7 100644
> > > > --- a/spice/vd_agent.h
> > > > +++ b/spice/vd_agent.h
> > > > @@ -218,6 +218,9 @@ typedef struct SPICE_ATTR_PACKED VDAgentClipboardGrab
> > > > {
> > > >  #if 0 /* VD_AGENT_CAP_CLIPBOARD_SELECTION */
> > > >      uint8_t selection;
> > > >      uint8_t __reserved[sizeof(uint32_t) - 1 * sizeof(uint8_t)];
> > > > +#endif
> > > > +#if 0 /* VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL */
> > > > +    uint32_t serial;
> > > >  #endif
> > >
> > > I would prefer a new message instead of partial structures.
> >
> > VDAgentClipboardGrabSelectionAndSerialAnd... ?
> >
>
> Usually a version schema is used like VDAgentClipboardGrabSelection_v2
>
> > yeah, it's not ideal. I wish we would use a better protocol, be it
> > json or protobuf etc..
> >
>
> This is not a justification to produce even worse code.
> A plain C structure would be fine, many binary protocols (like IP)
> uses this method.
> A semi-defined C structure with missing fields is the worst I
> ever seen, continuing to use is not so sensible.

Different trade-offs, version vs capabilities.

Since we don't have missing fields in the protocol, this means
VDAgentClipboardGrabSelection_v2 should have selection capability
included.

This may not be a problem for vdagent, but for many cases,
capabilities offer more flexibility than versioning.

> It would be better to not define the C structure at all.
> OT: I think it was discussed to use an external library/tool, somebody
> suggested to implement new messages with different serialization
> but nobody came with a RFC/proposal.

I guess we could start by having a gitlab issue to discuss it.

> >
> >
> > > Why not reusing part of __reserved?
> >
> > Because it's only 24 bits there, and if we make it too small, we would
> > have to deal explicitly with rounding issues.
> >
>
> I don't think a "(char)(remote_serial - local_serial) > 0" is so
> complicated to read.

I am not good at wrapped arithmetic. How would that work? for ex
(char)(255 - 100) == -101, that doesn't look right.

> But this is second to use a semi-defined structure, just a
> workaround using the already present ugly hack.
>
> > >
> > > >      uint32_t types[0];
> > > >  } VDAgentClipboardGrab;
> > > > @@ -288,6 +291,7 @@ enum {
> > > >      VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS,
> > > >      VD_AGENT_CAP_GRAPHICS_DEVICE_INFO,
> > > >      VD_AGENT_CAP_CLIPBOARD_NO_RELEASE_ON_REGRAB,
> > > > +    VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL,
> > > >      VD_AGENT_END_CAP,
> > > >  };
> > > >
> > >
> > > I lost the original issue. Won't be easier to just define a static
> > > precedence,
> > > like "in case of conflict the client win"? It would avoid to have 2 cases
> > > to test,
> > > each potentially with problems to solve.
> >
> > You mean without this request serial?
> >
> > How would you "legitimately steal" the client grab then?
> >
> >
> > thanks
> >
>
> I think I lost a bit all the cases which I suppose are not much
> documented.

The role of the grab message is to take ownership of the clipboard (to
advertize clipboard data available). It may come at any time from both
side, and override the current grab owner. It may come from the guest
(after C-c in guest app, for ex), so the client grabs the clipboard.
Or it may come from the client (C-c in client side app), and the agent
will grab the guest clipboard.

-- 
Marc-André Lureau