[Spice-devel] [spice-server v4 09/11] qxl: Add red_surface_cmd_{new, ref, unref} helpers

[Christophe Fergeau]

Currently, RedWorker is using stack-allocated variables for RedSurfaceCmd.
Surface commands are rare enough that we can dynamically allocate them
instead, and make the API in red-parse-qxl.h consistent with how other
QXL commands are handled.
---
 server/red-parse-qxl.c          | 38 ++++++++++++++++++++++++++++++---
 server/red-parse-qxl.h          |  8 ++++---
 server/red-worker.c             | 16 +++++++-------
 server/tests/test-qxl-parsing.c | 16 ++++++++------
 4 files changed, 58 insertions(+), 20 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 00ec8b9c9..736e66d97 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1411,8 +1411,8 @@ bool red_validate_surface(uint32_t width, uint32_t height,
     return true;
 }
 
-bool red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
-                         RedSurfaceCmd *red, QXLPHYSICAL addr)
+static bool red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
+                                RedSurfaceCmd *red, QXLPHYSICAL addr)
 {
     QXLSurfaceCmd *qxl;
     uint64_t size;
@@ -1451,11 +1451,43 @@ bool red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
     return true;
 }
 
-void red_put_surface_cmd(RedSurfaceCmd *red)
+static void red_put_surface_cmd(RedSurfaceCmd *red)
 {
     /* nothing yet */
 }
 
+RedSurfaceCmd *red_surface_cmd_new(QXLInstance *qxl_instance, RedMemSlotInfo *slots,
+                                   int group_id, QXLPHYSICAL addr)
+{
+    RedSurfaceCmd *cmd;
+
+    cmd = g_new0(RedSurfaceCmd, 1);
+
+    cmd->refs = 1;
+
+    if (!red_get_surface_cmd(slots, group_id, cmd, addr)) {
+        g_free(cmd);
+        return NULL;
+    }
+
+    return cmd;
+}
+
+RedSurfaceCmd *red_surface_cmd_ref(RedSurfaceCmd *cmd)
+{
+    cmd->refs++;
+    return cmd;
+}
+
+void red_surface_cmd_unref(RedSurfaceCmd *cmd)
+{
+    if (--cmd->refs) {
+        return;
+    }
+    red_put_surface_cmd(cmd);
+    g_free(cmd);
+}
+
 static bool red_get_cursor(RedMemSlotInfo *slots, int group_id,
                            SpiceCursor *red, QXLPHYSICAL addr)
 {
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index d969b3004..37e4ffb21 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -86,6 +86,7 @@ typedef struct RedSurfaceCreate {
 
 typedef struct RedSurfaceCmd {
     QXLReleaseInfoExt release_info_ext;
+    int refs;
     uint32_t surface_id;
     uint8_t type;
     uint32_t flags;
@@ -132,9 +133,10 @@ void red_message_unref(RedMessage *red);
 bool red_validate_surface(uint32_t width, uint32_t height,
                           int32_t stride, uint32_t format);
 
-bool red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
-                         RedSurfaceCmd *red, QXLPHYSICAL addr);
-void red_put_surface_cmd(RedSurfaceCmd *red);
+RedSurfaceCmd *red_surface_cmd_new(QXLInstance *qxl_instance, RedMemSlotInfo *slots,
+                                   int group_id, QXLPHYSICAL addr);
+RedSurfaceCmd *red_surface_cmd_ref(RedSurfaceCmd *cmd);
+void red_surface_cmd_unref(RedSurfaceCmd *cmd);
 
 RedCursorCmd *red_cursor_cmd_new(QXLInstance *qxl, RedMemSlotInfo *slots,
                                  int group_id, QXLPHYSICAL addr);
diff --git a/server/red-worker.c b/server/red-worker.c
index 7734fc04c..942eee571 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -152,16 +152,16 @@ static int red_process_cursor(RedWorker *worker, int *ring_is_empty)
 
 static gboolean red_process_surface_cmd(RedWorker *worker, QXLCommandExt *ext, gboolean loadvm)
 {
-    RedSurfaceCmd surface_cmd;
+    RedSurfaceCmd *surface_cmd;
 
-    if (!red_get_surface_cmd(&worker->mem_slots, ext->group_id, &surface_cmd, ext->cmd.data)) {
-        return FALSE;
+    surface_cmd = red_surface_cmd_new(worker->qxl, &worker->mem_slots, ext->group_id, ext->cmd.data);
+    if (surface_cmd == NULL) {
+        return false;
     }
-    display_channel_process_surface_cmd(worker->display_channel, &surface_cmd, loadvm);
-    // display_channel_process_surface_cmd() takes ownership of 'release_info_ext',
-    // we don't need to release it ourselves
-    red_put_surface_cmd(&surface_cmd);
-    return TRUE;
+    display_channel_process_surface_cmd(worker->display_channel, surface_cmd, loadvm);
+    red_surface_cmd_unref(surface_cmd);
+
+    return true;
 }
 
 static int red_process_display(RedWorker *worker, int *ring_is_empty)
diff --git a/server/tests/test-qxl-parsing.c b/server/tests/test-qxl-parsing.c
index e757bdfac..f244379bc 100644
--- a/server/tests/test-qxl-parsing.c
+++ b/server/tests/test-qxl-parsing.c
@@ -88,14 +88,16 @@ static void deinit_qxl_surface(QXLSurfaceCmd *qxl)
 static void test_no_issues(void)
 {
     RedMemSlotInfo mem_info;
-    RedSurfaceCmd cmd;
+    RedSurfaceCmd *cmd;
     QXLSurfaceCmd qxl;
 
     init_meminfo(&mem_info);
     init_qxl_surface(&qxl);
 
     /* try to create a surface with no issues, should succeed */
-    g_assert_true(red_get_surface_cmd(&mem_info, 0, &cmd, to_physical(&qxl)));
+    cmd = red_surface_cmd_new(NULL, &mem_info, 0, to_physical(&qxl));
+    g_assert_nonnull(cmd);
+    red_surface_cmd_unref(cmd);
 
     deinit_qxl_surface(&qxl);
     memslot_info_destroy(&mem_info);
@@ -104,7 +106,7 @@ static void test_no_issues(void)
 static void test_stride_too_small(void)
 {
     RedMemSlotInfo mem_info;
-    RedSurfaceCmd cmd;
+    RedSurfaceCmd *cmd;
     QXLSurfaceCmd qxl;
 
     init_meminfo(&mem_info);
@@ -115,7 +117,8 @@ static void test_stride_too_small(void)
      * This can be used to cause buffer overflows so refuse it.
      */
     qxl.u.surface_create.stride = 256;
-    g_assert_false(red_get_surface_cmd(&mem_info, 0, &cmd, to_physical(&qxl)));
+    cmd = red_surface_cmd_new(NULL, &mem_info, 0, to_physical(&qxl));
+    g_assert_null(cmd);
 
     deinit_qxl_surface(&qxl);
     memslot_info_destroy(&mem_info);
@@ -124,7 +127,7 @@ static void test_stride_too_small(void)
 static void test_too_big_image(void)
 {
     RedMemSlotInfo mem_info;
-    RedSurfaceCmd cmd;
+    RedSurfaceCmd *cmd;
     QXLSurfaceCmd qxl;
 
     init_meminfo(&mem_info);
@@ -140,7 +143,8 @@ static void test_too_big_image(void)
     qxl.u.surface_create.stride = 0x08000004 * 4;
     qxl.u.surface_create.width = 0x08000004;
     qxl.u.surface_create.height = 0x40000020;
-    g_assert_false(red_get_surface_cmd(&mem_info, 0, &cmd, to_physical(&qxl)));
+    cmd = red_surface_cmd_new(NULL, &mem_info, 0, to_physical(&qxl));
+    g_assert_null(cmd);
 
     deinit_qxl_surface(&qxl);
     memslot_info_destroy(&mem_info);
-- 
2.19.1