>
> Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
> ---
>
> Just a minor patch partly inspired by a patch from Frediano Ziglio.
> 5975a98a94e0 at git://people.freedesktop.org/~fziglio/spice-protocol
>
Thanks to take it
> The "client|server" comments bear verification: they're based on a
> comment in do_client_monitors() in vdagentd.c that implies
> VD_AGENT_MONITORS_CONFIG can be sent by either the client or server
> which I'm not sure is true.
>
I took a bit of time and grep(s) to check some information.
>
> spice/vd_agent.h | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> index 42ec77a..0662e44 100644
> --- a/spice/vd_agent.h
> +++ b/spice/vd_agent.h
> @@ -62,15 +62,22 @@ typedef struct SPICE_ATTR_PACKED VDAgentMessage {
> #define VD_AGENT_CLIPBOARD_MAX_SIZE_ENV "SPICE_CLIPBOARD_MAX_SIZE"
> #endif
>
> +
> +/* vdagentd socket messages and types */
I don't agree with this comment. These are the messages for the agent
from the server which could be embedded in spice protocol (client <-> server)
through "agent_data" message in the "MainChannel".
For instance on Windows there's neither vdagentd nor socket.
> +
Why that empty line?
Maybe better something more visible like
/*
* WHATEVER
*/
I would suggest
/*
* Messages and types for guest agent.
* These messages are sent through the virtio port "com.redhat.spice.0"
* (agent <-> server) or embedded in "agent_data" SPICE protocol message in
* the "MainChannel" (server <-> client)
*/
> enum {
> + /* server -> agent */
> VD_AGENT_MOUSE_STATE = 1,
This is right
> + /* client|server -> agent (acknowledged using VD_AGENT_REPLY) */
> VD_AGENT_MONITORS_CONFIG,
Not exactly, this is originated from the client and handled by either
client or server. Why single line? I would say
/* client -> agent|server.
* Acknowledged using VD_AGENT_REPLY /*
> + /* agent -> client|server */
> VD_AGENT_REPLY,
No, server does nothing with it, just
/* agent -> client */
> /* Set clipboard data (both directions).
> * Message comes with type and data.
> * See VDAgentClipboard structure.
> */
I have to say I like this style more, for instance there's a
comment for the related message (even if not hard to guess).
> VD_AGENT_CLIPBOARD,
> + /* client -> agent */
> VD_AGENT_DISPLAY_CONFIG,
Correct. Agent (at list on Windows) send a reply with VD_AGENT_REPLY.
> VD_AGENT_ANNOUNCE_CAPABILITIES,
> /* Asks to listen for clipboard changes (both directions).
> @@ -254,7 +261,7 @@ typedef struct SPICE_ATTR_PACKED VDAgentDeviceDisplayInfo
> {
> uint32_t monitor_id;
> uint32_t device_display_id;
> uint32_t device_address_len;
> - uint8_t device_address[0]; // a zero-terminated string
> + uint8_t device_address[0]; /* a zero-terminated string */
Not really strong about it.
> } VDAgentDeviceDisplayInfo;
>
>
> @@ -270,6 +277,9 @@ typedef struct SPICE_ATTR_PACKED
> VDAgentGraphicsDeviceInfo {
> VDAgentDeviceDisplayInfo display_info[0];
> } VDAgentGraphicsDeviceInfo;
>
> +
> +/* Capabilities definitions */
> +
> enum {
> VD_AGENT_CAP_MOUSE_STATE = 0,
> VD_AGENT_CAP_MONITORS_CONFIG,
Can I send an update to this patch ?
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel