[Spice-devel] [protocol] protocol: Add some comments to vd_agentd.h

[Frediano Ziglio]

> 
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
> 
> 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