[Spice-devel] [PATCH spice v3 5/7] server: Filter VD_AGENT_MONITORS_CONFIG
Hans de Goede
hdegoede at redhat.com
Wed Sep 12 07:03:35 PDT 2012
Hi,
On 09/12/2012 03:13 PM, Alon Levy wrote:
> If the guest supports client monitors config we pass it the
> VDAgentMonitorsConfig message via the
> QXLInterface::client_monitors_config api instead of via the vdagent.
> ---
> server/agent-msg-filter.c | 8 ++++++
> server/agent-msg-filter.h | 1 +
> server/reds.c | 65 +++++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 72 insertions(+), 2 deletions(-)
>
> diff --git a/server/agent-msg-filter.c b/server/agent-msg-filter.c
> index 7584b52..b48dd76 100644
> --- a/server/agent-msg-filter.c
> +++ b/server/agent-msg-filter.c
> @@ -24,6 +24,7 @@
> #include <string.h>
> #include "red_common.h"
> #include "agent-msg-filter.h"
> +#include "red_dispatcher.h"
>
> void agent_msg_filter_init(struct AgentMsgFilter *filter,
> int copy_paste, int discard_all)
> @@ -80,6 +81,13 @@ data_to_read:
> filter->result = AGENT_MSG_FILTER_DISCARD;
> }
> break;
> + case VD_AGENT_MONITORS_CONFIG:
> + if (red_dispatcher_use_client_monitors_config()) {
> + filter->result = AGENT_MSG_FILTER_MONITORS_CONFIG;
> + } else {
> + filter->result = AGENT_MSG_FILTER_OK;
> + }
> + break;
> default:
> filter->result = AGENT_MSG_FILTER_OK;
> }
> diff --git a/server/agent-msg-filter.h b/server/agent-msg-filter.h
> index ecccfc7..0132ad7 100644
> --- a/server/agent-msg-filter.h
> +++ b/server/agent-msg-filter.h
> @@ -28,6 +28,7 @@ enum {
> AGENT_MSG_FILTER_OK,
> AGENT_MSG_FILTER_DISCARD,
> AGENT_MSG_FILTER_PROTO_ERROR,
> + AGENT_MSG_FILTER_MONITORS_CONFIG,
> AGENT_MSG_FILTER_END
> };
>
> diff --git a/server/reds.c b/server/reds.c
> index 98b316d..5e5d55d 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -234,6 +234,16 @@ typedef struct SpiceCharDeviceStateItem {
> SpiceCharDeviceState *st;
> } SpiceCharDeviceStateItem;
>
> +/* Intermediate state for on going monitors config message from a single
> + * client, being passed to the guest */
> +typedef struct RedsClientMonitorsConfig {
> + MainChannelClient *mcc;
> + uint8_t *buffer;
> + int buffer_size;
> + int buffer_pos;
> + int last_part_size;
last_part_size is not read anywhere, so it can be removed...
> +} RedsClientMonitorsConfig;
> +
> typedef struct RedsState {
> int listen_socket;
> int secure_listen_socket;
> @@ -284,6 +294,8 @@ typedef struct RedsState {
> #endif
> int peer_minor_version;
> int allow_multiple_clients;
> +
> + RedsClientMonitorsConfig client_monitors_config;
> } RedsState;
>
> static RedsState *reds = NULL;
> @@ -1184,6 +1196,47 @@ void reds_release_agent_data_buffer(uint8_t *buf)
> dev_state->recv_from_client_buf_pushed = FALSE;
> }
>
> +static void reds_client_monitors_config_cleanup(void)
> +{
> + RedsClientMonitorsConfig *cmc = &reds->client_monitors_config;
> +
> + cmc->buffer_size = cmc->buffer_pos = 0;
> + free(cmc->buffer);
> + cmc->buffer = NULL;
> + cmc->last_part_size = 0;
> + cmc->mcc = NULL;
> +}
> +
> +static void reds_on_main_agent_monitors_config(
> + MainChannelClient *mcc, void *message, size_t size)
> +{
> + VDAgentMessage *msg_header;
> + VDAgentMonitorsConfig *monitors_config;
> + RedsClientMonitorsConfig *cmc = &reds->client_monitors_config;
> +
> + cmc->buffer_size += size;
> + cmc->buffer = realloc(cmc->buffer, cmc->buffer_size);
You must error check the realloc return here, or use a version which never fails.
> + cmc->mcc = mcc;
> + memcpy(cmc->buffer + cmc->buffer_pos, message, size);
You are forgetting to update buffer_pos here !!!
> + if (sizeof(VDAgentMessage) + sizeof(VDAgentMonitorsConfig) > cmc->buffer_size) {
Since you're checking msg_header->size below, you nonly need to check for
sizeof(VDAgentMessage) > cmc->buffer_size here. Or better simplify the
2 checks to:
msg_header = (VDAgentMessage *)cmc->buffer;
if (sizeof(VDAgentMessage) > cmc->buffer_size ||
msg_header->size > cmc->buffer_size - sizeof(VDAgentMessage)) {
spice_debug("not enough data yet.\n");
return;
}
> + spice_debug("not enough data yet. %d < %ld\n", cmc->buffer_size,
> + sizeof(VDAgentMessage) + sizeof(VDAgentMonitorsConfig));
> + return;
> + }
> + msg_header = (VDAgentMessage *)cmc->buffer;
> + if (msg_header->size > cmc->buffer_size - sizeof(VDAgentMessage)) {
> + spice_debug("not enough data yet. %ld < %d\n",
> + cmc->buffer_size - sizeof(VDAgentMessage),
> + msg_header->size);
> + return;
> + }
> + monitors_config = (VDAgentMonitorsConfig *)(cmc->buffer + sizeof(*msg_header));
> + spice_debug("%s: %d\n", __func__, monitors_config->num_of_monitors);
> + cmc->last_part_size = size;
> + red_dispatcher_client_monitors_config(monitors_config);
> + reds_client_monitors_config_cleanup();
> +}
> +
> void reds_on_main_agent_data(MainChannelClient *mcc, void *message, size_t size)
> {
> VDIPortState *dev_state = &reds->agent_state;
> @@ -1199,16 +1252,18 @@ void reds_on_main_agent_data(MainChannelClient *mcc, void *message, size_t size)
> break;
> case AGENT_MSG_FILTER_DISCARD:
> return;
> + case AGENT_MSG_FILTER_MONITORS_CONFIG:
> + reds_on_main_agent_monitors_config(mcc, message, size);
> + return;
> case AGENT_MSG_FILTER_PROTO_ERROR:
> reds_disconnect();
> return;
> }
> -
> // TODO - start tracking agent data per channel
> header = (VDIChunkHeader *)dev_state->recv_from_client_buf->buf;
The chunk below seems like a separate bugfix which should be in its own commit.
> header->port = VDP_CLIENT_PORT;
> header->size = size;
> - reds->agent_state.recv_from_client_buf->buf_used = sizeof(VDIChunkHeader) + size;
> + dev_state->recv_from_client_buf->buf_used = sizeof(VDIChunkHeader) + size;
>
> dev_state->recv_from_client_buf_pushed = TRUE;
> spice_char_device_write_buffer_add(reds->agent_state.base, dev_state->recv_from_client_buf);
> @@ -3980,6 +4035,12 @@ static int do_spice_init(SpiceCoreInterface *core_interface)
> inputs_init();
>
> reds->mouse_mode = SPICE_MOUSE_MODE_SERVER;
> +
> + reds->client_monitors_config.buffer = NULL;
> + reds->client_monitors_config.buffer_pos = 0;
> + reds->client_monitors_config.buffer_size = 0;
> + reds->client_monitors_config.last_part_size = 0;
> +
Why not simply call reds_client_monitors_config_cleanup() here ?
> reds->allow_multiple_clients = getenv(SPICE_DEBUG_ALLOW_MC_ENV) != NULL;
> if (reds->allow_multiple_clients) {
> spice_warning("spice: allowing multiple client connections (crashy)");
>
Regards,
Hans
More information about the Spice-devel
mailing list