Re: [PATCH spice-server 2/8] reds: Factor out a function to marshall VDAgentGraphicsDeviceInfo message

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Untested, but looks fine.

Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>



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




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]