On Thu, 2018-11-08 at 05:18 -0500, Frediano Ziglio wrote:
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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 ?
Ah, sorry, a mistake in renaming...
> > + 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;
> }
Good catch!
> > + 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)
Can't do this. The reason I'm using the formula is because it still is
at the other end. This is what the client sends in monitors_config and
mouse_position and those IDs will be used to access the hash table.
But the good thing is if we ever fix the ugly display_id, the graphics
device info from this patch can just be stored by a different key. For
now, have to use this.
> > + 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?
The somewhat ugly pointer arithmetics? It's used once on the server and
once in the vdagent, so can't share the code, but it's not so bad?
> > + }
> > +}
> > +
> > 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.
Right, I'll remove them.
> > +
> > 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.
Each new session agent should get the data sent again. At least that's
how I understand it and how it worked for me when testing a simple
login session -> user session case...
Thanks,
Lukas
> > + 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel