[Spice-devel] [PATCH] protocol: RFC: add clipboard selection capability

[Hans de Goede]

Hi,

On 02/19/2011 02:04 PM, Marc-André Lureau wrote:
> Hi
>
> On Sat, Feb 19, 2011 at 11:07 AM, Hans de Goede<hdegoede at redhat.com>  wrote:
>>> VDAgentClipboard* messages MUST be prepended with a uint8_t indicating
>>> which clipboard selection to operate.
>>
>> I'm not sure I like this part, I would rather see new structs to use
>> when this capability is present:
>>
>> For example a new VDAgentSelectionGrab to use instead of
>> VDAgentClipboardGrab
>> when both sides have the capability, which looks like this:
>>
>> typedef struct SPICE_ATTR_PACKED VDAgentSelectionGrab {
>>     uint32_t selection;
>>     uint32_t types[0];
>> } VDAgentClipboardGrab;
>>
>> Note I made the selection a uint32_t on purpose, this way the following
>> uint32_t's in the struct will not get unaligned by the packing.
>
> That's kind of annoying, because we duplicate structure and code patch
> for just an int... This is certainly a topic I would like to discuss
> more :)
>

I'm not necessarily against not using the struct in the code, some code
I've written for example also does not always use the struct. But having the
struct (with a comment which of the 2 structs to use when) in the spice-protocol
vd_agent.h header IMHO makes it much clearer how this works, then some
vague text about prepending in a commit message.

> Tbh, I am not fond of the "struct" based approach. It's not the first
> time I want to modify slightly the protocol, and it's not easy to do.
>
> My experience with networking code was the best when working on
> PulseAudio, the pa_tagstruct can contain various fields depending on
> version and capabilities (take a look at
> http://git.0pointer.de/?p=pulseaudio.git;a=blob;f=src/pulsecore/protocol-native.c)
>
> This is very convenient, and the various helper functions make it easy
> to put variable length strings or property list etc..
>
> However, I realize that having the protocol defined in struct format
> make it easier for other to implement, and can be used to generate
> code.

The spice agent protocol will likely go away in the future anyway, to
be replaced by a more generic qemu agent protocol. This is being discussed
on the (high volume) qemu-devel mailinglist. I think they are opting for
some form of (xml) rpc or some such atm. I'm not closely following
the discussion.


>> I took a look at your linux vdagent implementation for this, and it looks
>> good. Except that you seem to do the prepending with the selection
>> unconditionally, iow even when the other side does not have the
>> VD_AGENT_CAP_CLIPBOARD_SELECTION cap.
>>
>
> Nope, the vdagentd scrap the selection byte if the client doesn't
> support it. I tested with spicec (since I didn't patch the spicec
> client to support it)

Ah, good I missed that, but does that mean that when viewing a Linux
vm with the new agent with a windows client, every selection of
code in the vm, becomes a ctrl+c from the windows pov? I don't
think this is desirable. If the other side does not have the
selection capability, only clipboard messages related to
the clipboard atom should be send. Also ideally this would
be handled in the per session agent and not the vdagentd as this
means listening for X events, waking in session vdagent, building a
message, sending it through a socket, waking the vdagentd process,
only to at the end of it all discard this message.

So to me it seems better to just not listen for selection events
on the primary selection at all when the peer does not have this cap.

Note btw that you are free to change the protocol between
vdagentd and vdagent

Regards,

Hans