[Spice-devel] [PATCH spice-server 2/8] reds: Factor out a function to marshall VDAgentGraphicsDeviceInfo message
Frediano Ziglio
fziglio at redhat.com
Tue Feb 12 17:24:23 UTC 2019
> On Tue, 2019-02-12 at 04:05 -0500, Frediano Ziglio wrote:
> >
> > > Untested, but looks fine.
> > >
> > > Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
> > >
> >
> > 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.
>
> Yeah, I believe marshal is technically correct
> Jonathon
>
I did some grep+wc on our code... looks like we are changing the dictionary!
I think 90% or more of the occurrences are marshalL.
But we have to start fixing it somewhere, why not now?
Looks like both marshaler and marshaller are correct although the later
(marshaller) seems more accepted.
>
> >
> > 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 at redhat.com>
> > > > ---
> > > > 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(STR
> > > > EAM_
> > > > 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_DEVIC
> > > > E_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_DEVIC
> > > > E_IN
> > > > FO,
> > > > + spice_marshaller_get_to
> > > > tal_
> > > > 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.
> > > > */
> > >
> > >
>
>
More information about the Spice-devel
mailing list