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

Submitted by Frediano Ziglio on July 5, 2019, 3:47 p.m.

Details

Message ID 20190705154702.16371-1-fziglio@redhat.com
State Superseded
Headers show
Series "red-qxl: Better encapsulation of device display information" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio July 5, 2019, 3:47 p.m.
Do not expose multiple function to fetch different part of
internal structures.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 server/red-qxl.c | 36 +++++++++++++++++++++---------------
 server/red-qxl.h |  4 +---
 server/reds.c    | 19 +------------------
 3 files changed, 23 insertions(+), 36 deletions(-)

Patch hide | download patch | download mbox

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;
+    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

Comments

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@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