[Spice-devel] [PATCH spice-server 2/8] reds: Factor out a function to marshall VDAgentGraphicsDeviceInfo message
Jonathon Jongsma
jjongsma at redhat.com
Tue Feb 12 17:34:27 UTC 2019
On Tue, 2019-02-12 at 12:24 -0500, Frediano Ziglio wrote:
> > 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.
Yeah, it's a bit of an odd case because both "marshal" and "marshall"
are valid words (though the latter is more often a family name), so
they get mixed up a lot. To be honest, I don't mind too much whether
you decide to keep the wrong spelling for consistency or start changing
them to the 'correct' spelling.
But yes, "marshaller" and "marshalling" usually have a double L due to
the normal pronunciation rules of English. "Marshaler" looks like it
should be pronounced with a long A sound. English is dumb ;)
Jonathon
>
> >
> > >
> > > 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_D
> > > > > EVIC
> > > > > 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_D
> > > > > EVIC
> > > > > E_IN
> > > > > FO,
> > > > > + spice_marshaller_ge
> > > > > t_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