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@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. > > > > 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@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 > > > > > (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. > > > > > */ > > > > > > > > > > > > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel