[Spice-devel] [PATCH] vdagent: support announce_capabilities

[Hans de Goede]

Hi,

On 08/29/2010 07:27 PM, Alon Levy wrote:
>
> ---
>   vdagent/vdagent.cpp |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 files changed, 51 insertions(+), 1 deletions(-)
>
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index b1f47c0..8cba032 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -37,6 +37,8 @@ private:
>       VDAgent();
>       void input_desktop_message_loop();
>       bool handle_mouse_event(VDAgentMouseState* state);
> +    bool handle_announce_capabilities(
> +        VDAgentAnnounceCapabilities* announce_capabilities, uint32_t port);
>       bool handle_mon_config(VDAgentMonitorsConfig* mon_config, uint32_t port);
>       bool handle_clipboard(VDAgentClipboard* clipboard, uint32_t size);
>       bool handle_display_config(VDAgentDisplayConfig* display_config, uint32_t port);
> @@ -522,6 +524,48 @@ void VDAgent::load_display_setting()
>       _display_setting.load();
>   }
>
> +bool VDAgent::handle_announce_capabilities(
> +    VDAgentAnnounceCapabilities* announce_capabilities, uint32_t port)
> +{
> +    DWORD msg_size;
> +    VDPipeMessage* caps_pipe_msg;
> +    VDAgentMessage* caps_msg;
> +    VDAgentAnnounceCapabilities* caps;
> +
> +    /** Disable/enable any features that are not compatible with client capabilities.
> +     */
> +    vd_printf("got capabilities %X%X%X%X%X%X%X%X", announce_capabilities->caps[0],
> +        announce_capabilities->caps[1], announce_capabilities->caps[2],
> +        announce_capabilities->caps[3], announce_capabilities->caps[4],
> +        announce_capabilities->caps[5], announce_capabilities->caps[6],
> +        announce_capabilities->caps[7]);
> +

1) The capabilities of the client should be stored somewhere for later use
    (same goes for the client btw, that should store the agent caps somewhere)

2) This function should be split in one handling the receiving of announce_capabilities
    (handle_announce_capabilities) and one to send the agents own capabilities.
    (announce_capabilities).

3) I don't see why the agent should wait with receiving client capabilities before
    announcing its own. It should send its own capabilities directly after connect. This
    will become esp. important when the agent gains capabilities for new types of
    agent-messages send by the server. The server and client may be a different version.
    So even though the client may never send, and does not know how to handle receiving,
    announce_capabilities, the agent sending announce capabilities would still be useful
    for the server to figure out agent capabilities.

> +    /** announce our own capabilities.
> +     */
> +    msg_size = VD_MESSAGE_HEADER_SIZE + sizeof(VDAgentAnnounceCapabilities);
> +    caps_pipe_msg = (VDPipeMessage*)write_lock(msg_size);
> +    if (!caps_pipe_msg) {
> +        return false;
> +    }
> +
> +    caps_pipe_msg->type = VD_AGENT_COMMAND;
> +    caps_pipe_msg->opaque = port;
> +    caps_pipe_msg->size = sizeof(VDAgentMessage) + sizeof(VDAgentAnnounceCapabilities);
> +    caps_msg = (VDAgentMessage*)caps_pipe_msg->data;
> +    caps_msg->protocol = VD_AGENT_PROTOCOL;
> +    caps_msg->type = VD_AGENT_ANNOUNCE_CAPABILITIES;
> +    caps_msg->opaque = 0;
> +    caps_msg->size = sizeof(VDAgentReply);
> +    caps = (VDAgentAnnounceCapabilities*)caps_msg->data;
> +    memset(caps->caps, 0, sizeof(caps->caps));
> +    VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_DISPLAY_CONFIG);

Like with the previous patch, I think that the capability bits for all msg types the agent may
send / knows how to handle when receiving should be set.

> +    write_unlock(msg_size);
> +    if (!_pending_write) {
> +        write_completion(0, 0,&_pipe_state.write.overlap);
> +    }
> +    return true;
> +}
> +
>   bool VDAgent::handle_display_config(VDAgentDisplayConfig* display_config, uint32_t port)
>   {
>       DisplaySettingOptions disp_setting_opts;
> @@ -740,7 +784,13 @@ void VDAgent::dispatch_message(VDAgentMessage* msg, uint32_t port)
>           if (!a->handle_display_config((VDAgentDisplayConfig*)msg->data, port)) {
>               vd_printf("handle_display_config failed");
>               a->_running = false;
> -        }
> +        }
> +        break;
> +    case VD_AGENT_ANNOUNCE_CAPABILITIES:
> +        if (!a->handle_announce_capabilities((VDAgentAnnounceCapabilities*)msg->data, port)) {
> +            vd_printf("handle_announce_capabilities failed");
> +            a->_running = false;
> +        }
>           break;
>       default:
>           vd_printf("Unsupported message type %u size %u", msg->type, msg->size);

Regards,

Hans