[Spice-devel] [PATCH spice-server] red-qxl: Better encapsulation of device display information

[Snir Sheriber]

Hi,

On 7/5/19 6:47 PM, Frediano Ziglio wrote:
> Do not expose multiple function to fetch different part of
> internal structures.

shouldn't be "Do not expose multiple functions to fetch different parts of
internal structure." ?


>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>   server/red-qxl.c | 36 +++++++++++++++++++++---------------
>   server/red-qxl.h |  4 +---
>   server/reds.c    | 19 +------------------
>   3 files changed, 23 insertions(+), 36 deletions(-)
>
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index d40d3970d..843da7dfe 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -809,22 +809,28 @@ void spice_qxl_set_device_info(QXLInstance *instance,
>       reds_send_device_display_info(red_qxl_get_server(instance->st));
>   }
>   
> -const char* red_qxl_get_device_address(const QXLInstance *qxl)
> +uint32_t red_qxl_marshall_device_display_info(const QXLInstance *qxl, SpiceMarshaller *m)
>   {
> -    const QXLState *qxl_state = qxl->st;
> -    return qxl_state->device_address;
> -}
> -
> -const uint32_t* red_qxl_get_device_display_ids(const QXLInstance *qxl)
> -{
> -    const QXLState *qxl_state = qxl->st;
> -    return qxl_state->device_display_ids;
> -}
> -
> -size_t red_qxl_get_monitors_count(const QXLInstance *qxl)
> -{
> -    const QXLState *qxl_state = qxl->st;
> -    return qxl_state->monitors_count;
> +    uint32_t device_count = 0;
> +    const char *const device_address = qxl->st->device_address;

new line maybe

Other than that it makes sense and fine with me (a bit uncomfortable 
with doing marshaling in red-qxl though)

Snir.

> +    const size_t device_address_len = strlen(device_address) + 1;
> +    if (device_address_len == 1) {
> +        return 0;
> +    }
> +    for (size_t i = 0; i < qxl->st->monitors_count; ++i) {
> +        spice_marshaller_add_uint32(m, qxl->id);
> +        spice_marshaller_add_uint32(m, i);
> +        spice_marshaller_add_uint32(m, qxl->st->device_display_ids[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,
> +                qxl->st->device_display_ids[i]);
> +    }
> +    return device_count;
>   }
>   
>   void red_qxl_init(RedsState *reds, QXLInstance *qxl)
> diff --git a/server/red-qxl.h b/server/red-qxl.h
> index 947539482..5bb254adb 100644
> --- a/server/red-qxl.h
> +++ b/server/red-qxl.h
> @@ -40,9 +40,7 @@ void red_qxl_put_gl_scanout(QXLInstance *qxl, SpiceMsgDisplayGlScanoutUnix *scan
>   void red_qxl_gl_draw_async_complete(QXLInstance *qxl);
>   int red_qxl_check_qxl_version(QXLInstance *qxl, int major, int minor);
>   SpiceServer* red_qxl_get_server(QXLState *qxl);
> -const char* red_qxl_get_device_address(const QXLInstance *qxl);
> -const uint32_t* red_qxl_get_device_display_ids(const QXLInstance *qxl);
> -size_t red_qxl_get_monitors_count(const QXLInstance *qxl);
> +uint32_t red_qxl_marshall_device_display_info(const QXLInstance *qxl, SpiceMarshaller *m);
>   
>   /* Wrappers around QXLInterface vfuncs */
>   void red_qxl_get_init_info(QXLInstance *qxl, QXLDevInitInfo *info);
> diff --git a/server/reds.c b/server/reds.c
> index 7783ab0b4..817fdd423 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -920,24 +920,7 @@ void reds_marshall_device_display_info(RedsState *reds, SpiceMarshaller *m)
>   
>       // 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) {
> -            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]);
> -        }
> +        device_count += red_qxl_marshall_device_display_info(qxl, m);
>       }
>   
>       // add the stream devices to the message