red-parse-qxl: Use a base reference class for RedSurfaceCmd

Don't code manually reference counting for this structure

Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
Acked-by: Victor Toso <victortoso@redhat.com>

7 files changed, 39 insertions, 71 deletions

diff --git a/server/display-channel-private.h b/server/display-channel-private.h
index 03077de1..49026663 100644
--- a/server/display-channel-private.h
+++ b/server/display-channel-private.h
@@ -54,10 +54,10 @@ typedef struct RedSurface {
     //fix me - better handling here
     /* 'create_cmd' holds surface data through a pointer to guest memory, it
      * must be valid as long as the surface is valid */
-    RedSurfaceCmd *create_cmd;
+    red::shared_ptr<const RedSurfaceCmd> create_cmd;
     /* QEMU expects the guest data for the command to be valid as long as the
      * surface is valid */
-    RedSurfaceCmd *destroy_cmd;
+    red::shared_ptr<const RedSurfaceCmd> destroy_cmd;
 } RedSurface;
 
 typedef struct MonitorsConfig {
diff --git a/server/display-channel.cpp b/server/display-channel.cpp
index 52abe18f..00f2e861 100644
--- a/server/display-channel.cpp
+++ b/server/display-channel.cpp
@@ -241,14 +241,8 @@ void display_channel_surface_unref(DisplayChannel *display, uint32_t surface_id)
     spice_assert(surface->context.canvas);
 
     surface->context.canvas->ops->destroy(surface->context.canvas);
-    if (surface->create_cmd != nullptr) {
-        red_surface_cmd_unref(surface->create_cmd);
-        surface->create_cmd = nullptr;
-    }
-    if (surface->destroy_cmd != nullptr) {
-        red_surface_cmd_unref(surface->destroy_cmd);
-        surface->destroy_cmd = nullptr;
-    }
+    surface->create_cmd.reset();
+    surface->destroy_cmd.reset();
 
     region_destroy(&surface->draw_dirty_region);
     surface->context.canvas = nullptr;
@@ -2098,8 +2092,8 @@ void display_channel_create_surface(DisplayChannel *display, uint32_t surface_id
         }
         memset(data, 0, height*abs(stride));
     }
-    g_warn_if_fail(surface->create_cmd == nullptr);
-    g_warn_if_fail(surface->destroy_cmd == nullptr);
+    g_warn_if_fail(!surface->create_cmd);
+    g_warn_if_fail(!surface->destroy_cmd);
     ring_init(&surface->current);
     ring_init(&surface->current_list);
     ring_init(&surface->depend_on_me);
@@ -2230,8 +2224,8 @@ DisplayChannel::DisplayChannel(RedsState *reds,
 }
 
 void display_channel_process_surface_cmd(DisplayChannel *display,
-                                         RedSurfaceCmd *surface_cmd,
-                                         int loadvm)
+                                         red::shared_ptr<const RedSurfaceCmd> &&surface_cmd,
+                                         bool loadvm)
 {
     uint32_t surface_id;
     RedSurface *surface;
@@ -2266,7 +2260,7 @@ void display_channel_process_surface_cmd(DisplayChannel *display,
                                        reloaded_surface,
                                        // reloaded surfaces will be sent on demand
                                        !reloaded_surface);
-        surface->create_cmd = red_surface_cmd_ref(surface_cmd);
+        surface->create_cmd = surface_cmd;
         break;
     }
     case QXL_SURFACE_CMD_DESTROY:
@@ -2274,7 +2268,7 @@ void display_channel_process_surface_cmd(DisplayChannel *display,
             spice_warning("avoiding destroying a surface twice");
             break;
         }
-        surface->destroy_cmd = red_surface_cmd_ref(surface_cmd);
+        surface->destroy_cmd = surface_cmd;
         display_channel_destroy_surface(display, surface_id);
         break;
     default:
diff --git a/server/display-channel.h b/server/display-channel.h
index da8ae07f..eb25407c 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -124,14 +124,14 @@ void                       display_channel_destroy_surfaces          (DisplayCha
 void                       display_channel_process_draw              (DisplayChannel *display,
                                                                       RedDrawable *red_drawable,
                                                                       uint32_t process_commands_generation);
-void                       display_channel_process_surface_cmd       (DisplayChannel *display,
-                                                                      RedSurfaceCmd *surface_cmd,
-                                                                      int loadvm);
 void                       display_channel_gl_scanout                (DisplayChannel *display);
 void                       display_channel_gl_draw                   (DisplayChannel *display,
                                                                       SpiceMsgDisplayGlDraw *draw);
 void                       display_channel_gl_draw_done              (DisplayChannel *display);
 
+void display_channel_process_surface_cmd(DisplayChannel *display,
+                                         red::shared_ptr<const RedSurfaceCmd> &&surface_cmd,
+                                         bool loadvm);
 void display_channel_update_monitors_config(DisplayChannel *display, const QXLMonitorsConfig *config,
                                             uint16_t count, uint16_t max_allowed);
 void display_channel_set_monitors_config_to_primary(DisplayChannel *display);
diff --git a/server/red-parse-qxl.cpp b/server/red-parse-qxl.cpp
index 198179a3..66f6b5d7 100644
--- a/server/red-parse-qxl.cpp
+++ b/server/red-parse-qxl.cpp
@@ -1490,45 +1490,26 @@ static bool red_get_surface_cmd(QXLInstance *qxl_instance, RedMemSlotInfo *slots
     return true;
 }
 
-static void red_put_surface_cmd(RedSurfaceCmd *red)
+RedSurfaceCmd::~RedSurfaceCmd()
 {
-    if (red->qxl) {
-        red_qxl_release_resource(red->qxl, red->release_info_ext);
+    if (qxl) {
+        red_qxl_release_resource(qxl, release_info_ext);
     }
 }
 
-RedSurfaceCmd *red_surface_cmd_new(QXLInstance *qxl_instance, RedMemSlotInfo *slots,
-                                   int group_id, QXLPHYSICAL addr)
+red::shared_ptr<const RedSurfaceCmd>
+red_surface_cmd_new(QXLInstance *qxl_instance, RedMemSlotInfo *slots,
+                    int group_id, QXLPHYSICAL addr)
 {
-    RedSurfaceCmd *cmd;
-
-    cmd = g_new0(RedSurfaceCmd, 1);
+    auto cmd = red::make_shared<RedSurfaceCmd>();
 
-    cmd->refs = 1;
-
-    if (!red_get_surface_cmd(qxl_instance, slots, group_id, cmd, addr)) {
-        red_surface_cmd_unref(cmd);
-        return nullptr;
+    if (!red_get_surface_cmd(qxl_instance, slots, group_id, cmd.get(), addr)) {
+        cmd.reset();
     }
 
     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 d1aab512..1eb0d928 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -87,17 +87,17 @@ typedef struct RedSurfaceCreate {
     uint8_t *data;
 } RedSurfaceCreate;
 
-typedef struct RedSurfaceCmd {
+struct RedSurfaceCmd final: public red::simple_ptr_counted<RedSurfaceCmd> {
+    ~RedSurfaceCmd();
     QXLInstance *qxl;
     QXLReleaseInfoExt release_info_ext;
-    int refs;
     uint32_t surface_id;
     uint8_t type;
     uint32_t flags;
     union {
         RedSurfaceCreate surface_create;
     } u;
-} RedSurfaceCmd;
+};
 
 struct RedCursorCmd final: public red::simple_ptr_counted<RedCursorCmd> {
     ~RedCursorCmd();
@@ -139,10 +139,9 @@ void red_message_unref(RedMessage *red);
 bool red_validate_surface(uint32_t width, uint32_t height,
                           int32_t stride, uint32_t format);
 
-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);
+red::shared_ptr<const RedSurfaceCmd>
+red_surface_cmd_new(QXLInstance *qxl_instance, RedMemSlotInfo *slots,
+                    int group_id, QXLPHYSICAL addr);
 
 red::shared_ptr<const RedCursorCmd>
 red_cursor_cmd_new(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id, QXLPHYSICAL addr);
diff --git a/server/red-worker.cpp b/server/red-worker.cpp
index 9aad023b..62028a53 100644
--- a/server/red-worker.cpp
+++ b/server/red-worker.cpp
@@ -145,15 +145,12 @@ 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;
-
-    surface_cmd = red_surface_cmd_new(worker->qxl, &worker->mem_slots,
-                                      ext->group_id, ext->cmd.data);
-    if (surface_cmd == nullptr) {
+    auto surface_cmd = red_surface_cmd_new(worker->qxl, &worker->mem_slots,
+                                           ext->group_id, ext->cmd.data);
+    if (!surface_cmd) {
         return false;
     }
-    display_channel_process_surface_cmd(worker->display_channel, surface_cmd, loadvm);
-    red_surface_cmd_unref(surface_cmd);
+    display_channel_process_surface_cmd(worker->display_channel, std::move(surface_cmd), loadvm);
 
     return true;
 }
diff --git a/server/tests/test-qxl-parsing.cpp b/server/tests/test-qxl-parsing.cpp
index 510e275b..d68879f1 100644
--- a/server/tests/test-qxl-parsing.cpp
+++ b/server/tests/test-qxl-parsing.cpp
@@ -111,16 +111,15 @@ static void test_memslot_invalid_addresses(void)
 static void test_no_issues(void)
 {
     RedMemSlotInfo mem_info;
-    RedSurfaceCmd *cmd;
     QXLSurfaceCmd qxl;
 
     init_meminfo(&mem_info);
     init_qxl_surface(&qxl);
 
     /* try to create a surface with no issues, should succeed */
-    cmd = red_surface_cmd_new(NULL, &mem_info, 0, to_physical(&qxl));
-    g_assert_nonnull(cmd);
-    red_surface_cmd_unref(cmd);
+    auto cmd = red_surface_cmd_new(NULL, &mem_info, 0, to_physical(&qxl));
+    g_assert(cmd);
+    cmd.reset();
 
     deinit_qxl_surface(&qxl);
     memslot_info_destroy(&mem_info);
@@ -129,7 +128,6 @@ static void test_no_issues(void)
 static void test_stride_too_small(void)
 {
     RedMemSlotInfo mem_info;
-    RedSurfaceCmd *cmd;
     QXLSurfaceCmd qxl;
 
     init_meminfo(&mem_info);
@@ -140,8 +138,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;
-    cmd = red_surface_cmd_new(NULL, &mem_info, 0, to_physical(&qxl));
-    g_assert_null(cmd);
+    auto cmd = red_surface_cmd_new(NULL, &mem_info, 0, to_physical(&qxl));
+    g_assert(!cmd);
 
     deinit_qxl_surface(&qxl);
     memslot_info_destroy(&mem_info);
@@ -150,7 +148,6 @@ static void test_stride_too_small(void)
 static void test_too_big_image(void)
 {
     RedMemSlotInfo mem_info;
-    RedSurfaceCmd *cmd;
     QXLSurfaceCmd qxl;
 
     init_meminfo(&mem_info);
@@ -166,8 +163,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;
-    cmd = red_surface_cmd_new(NULL, &mem_info, 0, to_physical(&qxl));
-    g_assert_null(cmd);
+    auto cmd = red_surface_cmd_new(NULL, &mem_info, 0, to_physical(&qxl));
+    g_assert(!cmd);
 
     deinit_qxl_surface(&qxl);
     memslot_info_destroy(&mem_info);