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

Submitted by Frediano Ziglio on July 7, 2019, 4:35 p.m.

Details

Message ID 20190707163545.28466-1-fziglio@redhat.com
State Accepted
Commit 4f5c342eea5928a498e03efbff618c119e8d7a72
Headers show
Series "red-qxl: Better encapsulation of device display information" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio July 7, 2019, 4:35 p.m.
Do not expose multiple functions to fetch different parts of
internal structure.

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

Changes since v1:
- commit message fixes
- add new line after variable declarations
- add qxl_state variable

Patch hide | download patch | download mbox

diff --git a/server/red-qxl.c b/server/red-qxl.c
index d40d3970d..03348270f 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -809,22 +809,30 @@  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;
-}
+    uint32_t device_count = 0;
+    const char *const device_address = qxl_state->device_address;
+    const size_t device_address_len = strlen(device_address) + 1;
 
-size_t red_qxl_get_monitors_count(const QXLInstance *qxl)
-{
-    const QXLState *qxl_state = qxl->st;
-    return qxl_state->monitors_count;
+    if (device_address_len == 1) {
+        return 0;
+    }
+    for (size_t i = 0; i < qxl_state->monitors_count; ++i) {
+        spice_marshaller_add_uint32(m, qxl->id);
+        spice_marshaller_add_uint32(m, i);
+        spice_marshaller_add_uint32(m, qxl_state->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_state->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

Acked-by: Snir Sheriber <ssheribe@redhat.com>


On 7/7/19 7:35 PM, Frediano Ziglio wrote:
> Do not expose multiple functions to fetch different parts of
> internal structure.
>
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>   server/red-qxl.c | 34 +++++++++++++++++++++-------------
>   server/red-qxl.h |  4 +---
>   server/reds.c    | 19 +------------------
>   3 files changed, 23 insertions(+), 34 deletions(-)
>
> Changes since v1:
> - commit message fixes
> - add new line after variable declarations
> - add qxl_state variable
>
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index d40d3970d..03348270f 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -809,22 +809,30 @@ 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;
> -}
> +    uint32_t device_count = 0;
> +    const char *const device_address = qxl_state->device_address;
> +    const size_t device_address_len = strlen(device_address) + 1;
>   
> -size_t red_qxl_get_monitors_count(const QXLInstance *qxl)
> -{
> -    const QXLState *qxl_state = qxl->st;
> -    return qxl_state->monitors_count;
> +    if (device_address_len == 1) {
> +        return 0;
> +    }
> +    for (size_t i = 0; i < qxl_state->monitors_count; ++i) {
> +        spice_marshaller_add_uint32(m, qxl->id);
> +        spice_marshaller_add_uint32(m, i);
> +        spice_marshaller_add_uint32(m, qxl_state->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_state->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