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

[Frediano Ziglio]

> 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.
> 

I don't think this is an issue

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

Sure. What I don't like, if is not really clear is having a
semi-defined C structure. I would prefer to not having the structure
instead. There's no C rule to define "this field is present if you
have this capability", VDAgentClipboardGrabSelection does not
represent a message (unless there's no selection) but currently
the end of a message so it's misleading.

> > 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.
> 

By the way. It seems that the main libraries to do these job
(protobuf but also capnproto) are not C friendly.
Do we want to change language or use multiple languages?
If not I would personally stick/move to our serializing code
(the Python generator).

> > >
> > >
> > > > 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.
> 

If you thing 255 as -1 -> -1 - 100 = -101, that's expected.
Or, if you prefer to get 100 from 255 you need to add 101.
It's a pretty normal construct of tick computation, the signed
conversion is more to simply understand the before/after.
You expect small differences so if you get 255 and 100 you
have other issues.

> > 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.
> 

Yes, I realized this morning thinking about how clipboards works
(very rusty in my mind).
Instead of setting it you get the ownership that is you are willing
to set a value on the clipboard but you defer setting it to avoid
the expense of data copy.
So, the old (original?) protocol was simply to sending entire data
on every change, this avoided to loose the clipboard data entirely.
The current is: if we get new local clipboard data send the "grab"
so remote knows that will have to read our data.
So either we have ownership/grab, meaning the data are remote
waiting to get fetched or we don't have ownership and the remote
should be grabbing as we have data to send (at least in a stable
state).
Not sure what is the initial state, when you connect.
But from the stable state (one and only one has the ownership)
is not clear how you get both sending the grab message at the same
time (the one without ownership should not send the grab).

Frediano