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

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

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

yeah, it's not ideal. I wish we would use a better protocol, be it
json or protobuf etc..



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

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


-- 
Marc-André Lureau