> Untested, but looks fine.
>
> Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
>
It's partially tested by following patch. Partially as the new function is
tested but the old function to send the message is not but is changed in
this test.
But I can see data sent to the guest (so I tested that part manually).
Should the verb be "marshall" or "marshal" ? I think the second, so
I should rename the function.
Frediano
>
>
> On Mon, 2019-02-11 at 11:54 +0000, Frediano Ziglio wrote:
> > Instead of scanning the monitor twice (one to compute the size
> > and another to build the message) use a single function to
> > marshall the message.
> > This also fixes big endian machines (which are not supported).
> > Marshall function is exported to make easier to test (see following
> > patch).
> >
> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> > Also the reds_marshall_device_display_info function will be
> > potentially used to separate VDIPort code.
> > ---
> > server/reds.c | 143 ++++++++++++++++++++++------------------------
> > ----
> > server/reds.h | 3 ++
> > 2 files changed, 66 insertions(+), 80 deletions(-)
> >
> > diff --git a/server/reds.c b/server/reds.c
> > index 86f020a87..c16655adf 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -897,78 +897,33 @@ static RedPipeItem
> > *vdi_port_read_one_msg_from_device(RedCharDevice *self,
> > return NULL;
> > }
> >
> > -void reds_send_device_display_info(RedsState *reds)
> > +void reds_marshall_device_display_info(RedsState *reds,
> > SpiceMarshaller *m)
> > {
> > - if (!reds->agent_dev->priv->agent_attached) {
> > - return;
> > - }
> > - g_debug("Sending device display info to the agent:");
> > -
> > QXLInstance *qxl;
> > RedCharDevice *dev;
> >
> > - size_t message_size = sizeof(VDAgentGraphicsDeviceInfo);
> > -
> > - // size for the qxl device info
> > - FOREACH_QXL_INSTANCE(reds, qxl) {
> > - message_size +=
> > - (sizeof(VDAgentDeviceDisplayInfo) +
> > strlen(red_qxl_get_device_address(qxl)) + 1) *
> > - red_qxl_get_monitors_count(qxl);
> > - }
> > -
> > - // size for the stream device info
> > - GLIST_FOREACH(reds->char_devices, RedCharDevice, dev) {
> > - if (IS_STREAM_DEVICE(dev)) {
> > - size_t device_address_len =
> > - strlen(stream_device_get_device_display_info(STREAM_
> > DEVICE(dev))->device_address);
> > -
> > - if (device_address_len == 0) {
> > - // the device info wasn't set (yet), don't send it
> > - continue;
> > - }
> > -
> > - message_size += (sizeof(VDAgentDeviceDisplayInfo) +
> > device_address_len + 1);
> > - }
> > - }
> > -
> > - RedCharDeviceWriteBuffer *char_dev_buf =
> > vdagent_new_write_buffer(reds->agent_dev,
> > - VD_AGENT_GRAPHICS_DEVICE_IN
> > FO,
> > - message_size,
> > - true);
> > -
> > - if (!char_dev_buf) {
> > - reds->pending_device_display_info_message = true;
> > - return;
> > - }
> > -
> > - VDInternalBuf *internal_buf = (VDInternalBuf *)char_dev_buf-
> > >buf;
> > - VDAgentGraphicsDeviceInfo *graphics_device_info = &internal_buf-
> > >u.graphics_device_info;
> > - graphics_device_info->count = 0;
> > -
> > - VDAgentDeviceDisplayInfo *device_display_info =
> > graphics_device_info->display_info;
> > + uint32_t device_count = 0;
> > + void *device_count_ptr = spice_marshaller_add_uint32(m,
> > device_count);
> >
> > // add the qxl devices to the message
> > FOREACH_QXL_INSTANCE(reds, qxl) {
> > + const char *const device_address =
> > red_qxl_get_device_address(qxl);
> > + const size_t device_address_len = strlen(device_address) +
> > 1;
> > + if (device_address_len == 1) {
> > + continue;
> > + }
> > for (size_t i = 0; i < red_qxl_get_monitors_count(qxl); ++i)
> > {
> > - device_display_info->channel_id = qxl->id;
> > - device_display_info->monitor_id = i;
> > - device_display_info->device_display_id =
> > red_qxl_get_device_display_ids(qxl)[i];
> > -
> > - strcpy((char*) device_display_info->device_address,
> > red_qxl_get_device_address(qxl));
> > -
> > - device_display_info->device_address_len =
> > - strlen((char*) device_display_info->device_address)
> > + 1;
> > -
> > - g_debug(" (qxl) channel_id: %u monitor_id: %u,
> > device_address: %s, device_display_id: %u",
> > - device_display_info->channel_id,
> > - device_display_info->monitor_id,
> > - device_display_info->device_address,
> > - device_display_info->device_display_id);
> > -
> > - device_display_info = (VDAgentDeviceDisplayInfo*)
> > ((char*) device_display_info +
> > - sizeof(VDAgentDeviceDisplayInfo) +
> > device_display_info->device_address_len);
> > -
> > - graphics_device_info->count++;
> > + spice_marshaller_add_uint32(m, qxl->id);
> > + spice_marshaller_add_uint32(m, i);
> > + spice_marshaller_add_uint32(m,
> > red_qxl_get_device_display_ids(qxl)[i]);
> > + spice_marshaller_add_uint32(m, device_address_len);
> > + spice_marshaller_add(m, (void*) device_address,
> > device_address_len);
> > + ++device_count;
> > +
> > + g_debug(" (qxl) channel_id: %u monitor_id: %zu,
> > device_address: %s, "
> > + "device_display_id: %u",
> > + qxl->id, i, device_address,
> > + red_qxl_get_device_display_ids(qxl)[i]);
> > }
> > }
> >
> > @@ -977,9 +932,9 @@ void reds_send_device_display_info(RedsState
> > *reds)
> > if (IS_STREAM_DEVICE(dev)) {
> > StreamDevice *stream_dev = STREAM_DEVICE(dev);
> > const StreamDeviceDisplayInfo *info =
> > stream_device_get_device_display_info(stream_dev);
> > - size_t device_address_len = strlen(info-
> > >device_address);
> > + size_t device_address_len = strlen(info->device_address)
> > + 1;
> >
> > - if (device_address_len == 0) {
> > + if (device_address_len == 1) {
> > // the device info wasn't set (yet), don't send it
> > continue;
> > }
> > @@ -990,25 +945,53 @@ void reds_send_device_display_info(RedsState
> > *reds)
> > continue;
> > }
> >
> > - device_display_info->channel_id = channel_id;
> > - device_display_info->monitor_id = info->stream_id;
> > - device_display_info->device_display_id = info-
> > >device_display_id;
> > + spice_marshaller_add_uint32(m, channel_id);
> > + spice_marshaller_add_uint32(m, info->stream_id);
> > + spice_marshaller_add_uint32(m, info->device_display_id);
> > + spice_marshaller_add_uint32(m, device_address_len);
> > + spice_marshaller_add(m, (void*) info->device_address,
> > device_address_len);
> > + ++device_count;
> > +
> > + g_debug(" (stream) channel_id: %u monitor_id: %u,
> > device_address: %s, "
> > + "device_display_id: %u",
> > + channel_id, info->stream_id, info-
> > >device_address,
> > + info->device_display_id);
> > + }
> > + }
> > + spice_marshaller_set_uint32(m, device_count_ptr, device_count);
> > +}
> >
> > - strcpy((char*) device_display_info->device_address,
> > info->device_address);
> > - device_display_info->device_address_len =
> > device_address_len + 1;
> > +void reds_send_device_display_info(RedsState *reds)
> > +{
> > + if (!reds->agent_dev->priv->agent_attached) {
> > + return;
> > + }
> > + g_debug("Sending device display info to the agent:");
> >
> > - g_debug(" (stream) channel_id: %u monitor_id: %u,
> > device_address: %s, device_display_id: %u",
> > - device_display_info->channel_id,
> > - device_display_info->monitor_id,
> > - device_display_info->device_address,
> > - device_display_info->device_display_id);
> > + SpiceMarshaller *m = spice_marshaller_new();
> > + reds_marshall_device_display_info(reds, m);
> >
> > - device_display_info = (VDAgentDeviceDisplayInfo*)
> > ((char*) device_display_info +
> > - sizeof(VDAgentDeviceDisplayInfo) +
> > device_display_info->device_address_len);
> > + RedCharDeviceWriteBuffer *char_dev_buf =
> > vdagent_new_write_buffer(reds->agent_dev,
> > + VD_AGENT_GRAPHICS_DEVICE_IN
> > FO,
> > + spice_marshaller_get_total_
> > size(m),
> > + true);
> >
> > - graphics_device_info->count++;
> > - }
> > + if (!char_dev_buf) {
> > + spice_marshaller_destroy(m);
> > + reds->pending_device_display_info_message = true;
> > + return;
> > + }
> > +
> > + VDInternalBuf *internal_buf = (VDInternalBuf *)char_dev_buf-
> > >buf;
> > +
> > + int free_info;
> > + size_t len_info;
> > + uint8_t *info = spice_marshaller_linearize(m, 0, &len_info,
> > &free_info);
> > + memcpy(&internal_buf->u.graphics_device_info, info, len_info);
> > + if (free_info) {
> > + free(info);
> > }
> > + spice_marshaller_destroy(m);
> >
> > reds->pending_device_display_info_message = false;
> >
> > diff --git a/server/reds.h b/server/reds.h
> > index e3641fa17..8c5ee84d0 100644
> > --- a/server/reds.h
> > +++ b/server/reds.h
> > @@ -100,6 +100,9 @@ SpiceCoreInterfaceInternal*
> > reds_get_core_interface(RedsState *reds);
> > void reds_update_client_mouse_allowed(RedsState *reds);
> > MainDispatcher* reds_get_main_dispatcher(RedsState *reds);
> >
> > +/* Marshal VDAgentGraphicsDeviceInfo structure */
> > +void reds_marshall_device_display_info(RedsState *reds,
> > SpiceMarshaller *m);
> > +
> > /* Get the recording object stored in RedsState.
> > * You should free with red_record_unref.
> > */
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel