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

[Frediano Ziglio]

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

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

Frediano