[Spice-devel] [RFC PATCH vd_agent 3/3] Receive the graphics_device_info message
Frediano Ziglio
fziglio at redhat.com
Thu Nov 8 10:18:06 UTC 2018
>
> The graphics_device_info message contains the device display ID
> information (device address and device display ID). Stores the data in a
> hash table in vdagent.
>
> Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> ---
> src/vdagent/vdagent.c | 69 ++++++++++++++++++++++++++++++++++++
> src/vdagentd-proto-strings.h | 1 +
> src/vdagentd-proto.h | 1 +
> src/vdagentd/vdagentd.c | 16 +++++++++
> 4 files changed, 87 insertions(+)
>
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index f7c8b72..d1af57f 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -53,6 +53,7 @@ typedef struct VDAgent {
> struct vdagent_file_xfers *xfers;
> struct udscs_connection *conn;
> GIOChannel *x11_channel;
> + GHashTable *graphics_display_infos;
>
> GMainLoop *loop;
> } VDAgent;
> @@ -95,6 +96,16 @@ static GOptionEntry entries[] = {
> { NULL }
> };
>
> +typedef struct GraphicsDisplayInfo {
Why this name went from GraphicsDevice to GraphicsDisplay ?
> + char device_address[256];
> + uint32_t device_display_id;
> +} GraphicsDisplayInfo;
> +
> +static void graphics_display_info_destroy(gpointer gdi)
> +{
> + g_free(gdi);
> +}
> +
> /**
> * xfer_get_download_directory
> *
> @@ -159,6 +170,55 @@ static gboolean vdagent_finalize_file_xfer(VDAgent
> *agent)
> return TRUE;
> }
>
> +static void vdagent_handle_graphics_device_info(VDAgent *agent, uint8_t
> *data, size_t size)
> +{
> + VDAgentGraphicsDeviceInfos *graphics_device_infos =
> (VDAgentGraphicsDeviceInfos *)data;
> + VDAgentGraphicsDeviceInfo *graphics_device_info =
> graphics_device_infos->graphics_device_infos;
> +
> + void *buffer_end = data + size;
> +
> + syslog(LOG_INFO, "Received Graphics Device Info:");
> +
> + for (size_t i = 0; i < graphics_device_infos->count; ++i) {
> + if ((void*) graphics_device_info > buffer_end ||
> + (void*) (&graphics_device_info->device_address +
> + graphics_device_info->device_address_len) > buffer_end)
> {
> + syslog(LOG_ERR, "Malformed graphics_display_info message, "
> + "extends beyond the end of the buffer");
> + break;
> + }
> +
> + syslog(LOG_INFO, " channel_id: %u monitor_id: %u device_address:
> %s, "
> + "device_display_id: %u",
> + graphics_device_info->channel_id,
> + graphics_device_info->monitor_id,
> + graphics_device_info->device_address,
> + graphics_device_info->device_display_id);
> +
> + GraphicsDisplayInfo *value = g_malloc(sizeof(GraphicsDisplayInfo));
You can use g_new.
> +
> + size_t device_address_len =
> graphics_device_info->device_address_len;
> + if (device_address_len > sizeof(value->device_address)) {
> + syslog(LOG_ERR, "Received a device address longer than %lu, "
> + "will be truncated!", device_address_len);
> + device_address_len = sizeof(value->device_address);
> + }
> +
> + strncpy(value->device_address,
> + (char*) graphics_device_info->device_address,
> + device_address_len);
> + value->device_address[device_address_len] = '\0'; // make sure the
> string is terminated
Can overflow if device_address_len == sizeof(value->device_address), will
overflow on the ID which probably will contain a 0 inside so not a big deal
but still overflow. Better to write the above condition as:
if (device_address_len >= sizeof(value->device_address)) {
syslog(LOG_ERR, "Received a device address longer than %lu, "
"will be truncated!", device_address_len);
device_address_len = sizeof(value->device_address) - 1;
}
> + value->device_display_id = graphics_device_info->device_display_id;
> +
> + g_hash_table_insert(agent->graphics_display_infos,
> + GUINT_TO_POINTER(graphics_device_info->channel_id +
> graphics_device_info->monitor_id),
Still this ugly formula? Surely channel_id is limited to 8 bit as a protocol
limitation, I would go with
graphics_device_info->channel_id + (graphics_device_info->monitor_id << 8)
Or use g_int64_equal/g_int64_hash instead and
graphics_device_info->channel_id + (graphics_device_info->monitor_id << 32)
> + value);
> +
> + graphics_device_info = (VDAgentGraphicsDeviceInfo*) ((char*)
> graphics_device_info +
> + sizeof(VDAgentGraphicsDeviceInfo) +
> graphics_device_info->device_address_len);
OT: this formula is repeated multiple times, maybe adding a macro would
help?
> + }
> +}
> +
> static void vdagent_quit_loop(VDAgent *agent)
> {
> /* other GMainLoop(s) might be running, quit them before agent->loop */
> @@ -243,6 +303,9 @@ static void daemon_read_complete(struct udscs_connection
> **connp,
> ((VDAgentFileXferDataMessage
> *)data)->id);
> }
> break;
> + case VDAGENTD_GRAPHICS_DEVICE_INFO:
> + vdagent_handle_graphics_device_info(agent, data, header->size);
> + break;
> case VDAGENTD_CLIENT_DISCONNECTED:
> vdagent_clipboards_release_all(agent->clipboards);
> if (vdagent_finalize_file_xfer(agent)) {
> @@ -345,6 +408,11 @@ static VDAgent *vdagent_new(void)
> g_unix_signal_add(SIGHUP, vdagent_signal_handler, agent);
> g_unix_signal_add(SIGTERM, vdagent_signal_handler, agent);
>
> + agent->graphics_display_infos = g_hash_table_new_full(&g_direct_hash,
> + &g_direct_equal,
> + NULL,
> +
> &graphics_display_info_destroy);
Why "&" before functions? This seems not coherent with current code.
> +
> return agent;
> }
>
> @@ -359,6 +427,7 @@ static void vdagent_destroy(VDAgent *agent)
>
> g_clear_pointer(&agent->x11_channel, g_io_channel_unref);
> g_clear_pointer(&agent->loop, g_main_loop_unref);
> + g_hash_table_destroy(agent->graphics_display_infos);
> g_free(agent);
> }
>
> diff --git a/src/vdagentd-proto-strings.h b/src/vdagentd-proto-strings.h
> index 6e7bcee..7e03f46 100644
> --- a/src/vdagentd-proto-strings.h
> +++ b/src/vdagentd-proto-strings.h
> @@ -36,6 +36,7 @@ static const char * const vdagentd_messages[] = {
> "file xfer data",
> "file xfer disable",
> "client disconnected",
> + "graphics device info",
> };
>
> #endif
> diff --git a/src/vdagentd-proto.h b/src/vdagentd-proto.h
> index f72a890..243a9c6 100644
> --- a/src/vdagentd-proto.h
> +++ b/src/vdagentd-proto.h
> @@ -44,6 +44,7 @@ enum {
> VDAGENTD_FILE_XFER_DATA,
> VDAGENTD_FILE_XFER_DISABLE,
> VDAGENTD_CLIENT_DISCONNECTED, /* daemon -> client */
> + VDAGENTD_GRAPHICS_DEVICE_INFO, /* daemon -> client */
> VDAGENTD_NO_MESSAGES /* Must always be last */
> };
>
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 99683da..7c37358 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -375,6 +375,16 @@ static void do_client_file_xfer(struct
> vdagent_virtio_port *vport,
> udscs_write(conn, msg_type, 0, 0, data, message_header->size);
> }
>
> +static void forward_data_to_session_agent(uint32_t type, uint8_t *data,
> size_t size)
> +{
> + if (active_session_conn == NULL) {
> + syslog(LOG_DEBUG, "No active session, can't forward message (type
> %u)", type);
Won't be need to cache this value and forward also to new sessions?
The new session should know the current values.
> + return;
> + }
> +
> + udscs_write(active_session_conn, type, 0, 0, data, size);
> +}
> +
> static gsize vdagent_message_min_size[] =
> {
> -1, /* Does not exist */
> @@ -393,6 +403,7 @@ static gsize vdagent_message_min_size[] =
> 0, /* VD_AGENT_CLIENT_DISCONNECTED */
> sizeof(VDAgentMaxClipboard), /* VD_AGENT_MAX_CLIPBOARD */
> sizeof(VDAgentAudioVolumeSync), /* VD_AGENT_AUDIO_VOLUME_SYNC */
> + sizeof(VDAgentGraphicsDeviceInfo), /* VD_AGENT_GRAPHICS_DEVICE_INFO */
> };
>
> static void vdagent_message_clipboard_from_le(VDAgentMessage
> *message_header,
> @@ -477,6 +488,7 @@ static gboolean vdagent_message_check_size(const
> VDAgentMessage *message_header)
> case VD_AGENT_CLIPBOARD_GRAB:
> case VD_AGENT_AUDIO_VOLUME_SYNC:
> case VD_AGENT_ANNOUNCE_CAPABILITIES:
> + case VD_AGENT_GRAPHICS_DEVICE_INFO:
> if (message_header->size < min_size) {
> syslog(LOG_ERR, "read: invalid message size: %u for message
> type: %u",
> message_header->size, message_header->type);
> @@ -550,6 +562,10 @@ static int virtio_port_read_complete(
> syslog(LOG_DEBUG, "Set max clipboard: %d", max_clipboard);
> break;
> }
> + case VD_AGENT_GRAPHICS_DEVICE_INFO: {
> + forward_data_to_session_agent(VDAGENTD_GRAPHICS_DEVICE_INFO, data,
> message_header->size);
> + break;
> + }
> case VD_AGENT_AUDIO_VOLUME_SYNC: {
> VDAgentAudioVolumeSync *vdata = (VDAgentAudioVolumeSync *)data;
> virtio_msg_uint16_from_le((uint8_t *)vdata, message_header->size,
Frediano
More information about the Spice-devel
mailing list