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

Don't code manually reference counting for this structure

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

server/Makefile.am

@@ -157,7 +157,7 @@ libserver_la_SOURCES =				\
 	red-pipe-item.h				\
 	red-qxl.cpp				\
 	red-qxl.h				\
-	red-record-qxl.c			\
+	red-record-qxl.cpp			\
 	red-record-qxl.h			\
 	red-replay-qxl.cpp			\
 	reds.cpp				\

server/cursor-channel.cpp

@@ -26,37 +26,24 @@
 #include "reds.h"
 
 struct RedCursorPipeItem: public RedPipeItemNum<RED_PIPE_ITEM_TYPE_CURSOR> {
-    explicit RedCursorPipeItem(RedCursorCmd *cmd);
-    ~RedCursorPipeItem() override;
-    RedCursorCmd *red_cursor;
+    explicit RedCursorPipeItem(const red::shared_ptr<const RedCursorCmd>& cmd);
+    red::shared_ptr<const RedCursorCmd> red_cursor;
 };
 
-RedCursorPipeItem::RedCursorPipeItem(RedCursorCmd *cmd):
-    red_cursor(red_cursor_cmd_ref(cmd))
+RedCursorPipeItem::RedCursorPipeItem(const red::shared_ptr<const RedCursorCmd>& cmd):
+    red_cursor(cmd)
 {
 }
 
-RedCursorPipeItem::~RedCursorPipeItem()
-{
-    red_cursor_cmd_unref(red_cursor);
-}
-
-static void cursor_channel_set_item(CursorChannel *cursor, RedCursorPipeItem *item)
-{
-    cursor->item.reset(item);
-}
-
 static void cursor_fill(CursorChannelClient *ccc, RedCursorPipeItem *cursor,
                         SpiceCursor *red_cursor, SpiceMarshaller *m)
 {
-    RedCursorCmd *cursor_cmd;
-
     if (!cursor) {
         red_cursor->flags = SPICE_CURSOR_FLAGS_NONE;
         return;
     }
 
-    cursor_cmd = cursor->red_cursor;
+    auto cursor_cmd = cursor->red_cursor.get();
     *red_cursor = cursor_cmd->u.set.shape;
 
     if (red_cursor->header.unique) {
@@ -100,11 +87,10 @@ static void red_marshall_cursor(CursorChannelClient *ccc,
 {
     CursorChannel *cursor_channel = ccc->get_channel();
     RedCursorPipeItem *item = cursor_pipe_item;
-    RedCursorCmd *cmd;
 
     spice_return_if_fail(cursor_channel);
 
-    cmd = item->red_cursor;
+    auto cmd = item->red_cursor.get();
     switch (cmd->type) {
     case QXL_CURSOR_MOVE:
         {
@@ -189,7 +175,7 @@ cursor_channel_new(RedsState *server, int id,
     return red::make_shared<CursorChannel>(server, id, core, dispatcher);
 }
 
-void CursorChannel::process_cmd(RedCursorCmd *cursor_cmd)
+void CursorChannel::process_cmd(red::shared_ptr<const RedCursorCmd> &&cursor_cmd)
 {
     bool cursor_show = false;
 
@@ -200,7 +186,7 @@ void CursorChannel::process_cmd(RedCursorCmd *cursor_cmd)
     switch (cursor_cmd->type) {
     case QXL_CURSOR_SET:
         cursor_visible = !!cursor_cmd->u.set.visible;
-        cursor_channel_set_item(this, cursor_pipe_item.get());
+        item = cursor_pipe_item;
         break;
     case QXL_CURSOR_MOVE:
         cursor_show = !cursor_visible;
@@ -229,7 +215,7 @@ void CursorChannel::process_cmd(RedCursorCmd *cursor_cmd)
 
 void CursorChannel::reset()
 {
-    cursor_channel_set_item(this, nullptr);
+    item.reset();
     cursor_visible = true;
     cursor_position.x = cursor_position.y = 0;
     cursor_trail_length = cursor_trail_frequency = 0;

server/cursor-channel.h

@@ -38,7 +38,7 @@ struct CursorChannel final: public CommonGraphicsChannel
     ~CursorChannel();
     void reset();
     void do_init();
-    void process_cmd(RedCursorCmd *cursor_cmd);
+    void process_cmd(red::shared_ptr<const RedCursorCmd> &&cursor_cmd);
     void set_mouse_mode(uint32_t mode);
     void on_connect(RedClient *client, RedStream *stream, int migration,
                     RedChannelCapabilities *caps) override;

server/meson.build

@@ -136,7 +136,7 @@ spice_server_sources = [
   'red-pipe-item.h',
   'red-qxl.cpp',
   'red-qxl.h',
-  'red-record-qxl.c',
+  'red-record-qxl.cpp',
   'red-record-qxl.h',
   'red-replay-qxl.cpp',
   'reds.cpp',

server/red-parse-qxl.cpp

@@ -1609,46 +1609,26 @@ static bool red_get_cursor_cmd(QXLInstance *qxl_instance, RedMemSlotInfo *slots,
     return true;
 }
 
-RedCursorCmd *red_cursor_cmd_new(QXLInstance *qxl, 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)
 {
-    RedCursorCmd *cmd;
+    auto cmd = red::make_shared<RedCursorCmd>();
 
-    cmd = g_new0(RedCursorCmd, 1);
-
-    cmd->refs = 1;
-
-    if (!red_get_cursor_cmd(qxl, slots, group_id, cmd, addr)) {
-        red_cursor_cmd_unref(cmd);
-        return nullptr;
+    if (!red_get_cursor_cmd(qxl, slots, group_id, cmd.get(), addr)) {
+        cmd.reset();
     }
 
     return cmd;
 }
 
-static void red_put_cursor_cmd(RedCursorCmd *red)
+RedCursorCmd::~RedCursorCmd()
 {
-    switch (red->type) {
+    switch (type) {
     case QXL_CURSOR_SET:
-        red_put_cursor(&red->u.set.shape);
+        red_put_cursor(&u.set.shape);
         break;
     }
-    if (red->qxl) {
-        red_qxl_release_resource(red->qxl, red->release_info_ext);
+    if (qxl) {
+        red_qxl_release_resource(qxl, release_info_ext);
     }
 }
-
-RedCursorCmd *red_cursor_cmd_ref(RedCursorCmd *red)
-{
-    red->refs++;
-    return red;
-}
-
-void red_cursor_cmd_unref(RedCursorCmd *red)
-{
-    if (--red->refs) {
-        return;
-    }
-    red_put_cursor_cmd(red);
-    g_free(red);
-}

server/red-parse-qxl.h

@@ -23,8 +23,9 @@
 
 #include "red-common.h"
 #include "memslot.h"
+#include "utils.hpp"
 
-SPICE_BEGIN_DECLS
+#include "push-visibility.h"
 
 typedef struct RedDrawable {
     int refs;
@@ -98,10 +99,10 @@ typedef struct RedSurfaceCmd {
     } u;
 } RedSurfaceCmd;
 
-typedef struct RedCursorCmd {
+struct RedCursorCmd final: public red::simple_ptr_counted<RedCursorCmd> {
+    ~RedCursorCmd();
     QXLInstance *qxl;
     QXLReleaseInfoExt release_info_ext;
-    int refs;
     uint8_t type;
     union {
         struct {
@@ -115,7 +116,7 @@ typedef struct RedCursorCmd {
         } trail;
         SpicePoint16 position;
     } u;
-} RedCursorCmd;
+};
 
 void red_get_rect_ptr(SpiceRect *red, const QXLRect *qxl);
 
@@ -143,11 +144,9 @@ RedSurfaceCmd *red_surface_cmd_new(QXLInstance *qxl_instance, RedMemSlotInfo *sl
 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);
-RedCursorCmd *red_cursor_cmd_ref(RedCursorCmd *red);
-void red_cursor_cmd_unref(RedCursorCmd *red);
+red::shared_ptr<const RedCursorCmd>
+red_cursor_cmd_new(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id, QXLPHYSICAL addr);
 
-SPICE_END_DECLS
+#include "pop-visibility.h"
 
 #endif /* RED_PARSE_QXL_H_ */

server/red-stream-device.cpp

@@ -378,10 +378,10 @@ get_cursor_type_bits(unsigned int cursor_type)
     }
 }
 
-static RedCursorCmd *
+static red::shared_ptr<const RedCursorCmd>
 stream_msg_cursor_set_to_cursor_cmd(const StreamMsgCursorSet *msg, size_t msg_size)
 {
-    auto cmd = g_new0(RedCursorCmd, 1);
+    auto cmd = red::make_shared<RedCursorCmd>();
     cmd->type = QXL_CURSOR_SET;
     cmd->u.set.position.x = 0; // TODO
     cmd->u.set.position.y = 0; // TODO
@@ -397,14 +397,12 @@ stream_msg_cursor_set_to_cursor_cmd(const StreamMsgCursorSet *msg, size_t msg_si
     /* Limit cursor size to prevent DoS */
     if (cursor->header.width > STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
         cursor->header.height > STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
-        g_free(cmd);
-        return nullptr;
+        return red::shared_ptr<const RedCursorCmd>();
     }
 
     const unsigned int cursor_bits = get_cursor_type_bits(cursor->header.type);
     if (cursor_bits == 0) {
-        g_free(cmd);
-        return nullptr;
+        return red::shared_ptr<const RedCursorCmd>();
     }
 
     /* Check that enough data has been sent for the cursor.
@@ -413,8 +411,7 @@ stream_msg_cursor_set_to_cursor_cmd(const StreamMsgCursorSet *msg, size_t msg_si
     size_t size_required = cursor->header.width * cursor->header.height;
     size_required = SPICE_ALIGN(size_required * cursor_bits, 8) / 8U;
     if (msg_size < sizeof(StreamMsgCursorSet) + size_required) {
-        g_free(cmd);
-        return nullptr;
+        return red::shared_ptr<const RedCursorCmd>();
     }
     cursor->data_size = size_required;
     cursor->data = (uint8_t*) g_memdup2(msg->data, size_required);
@@ -453,11 +450,11 @@ StreamDevice::handle_msg_cursor_set()
     }
 
     // transform the message to a cursor command and process it
-    RedCursorCmd *cmd = stream_msg_cursor_set_to_cursor_cmd(&msg->cursor_set, msg_pos);
+    auto cmd = stream_msg_cursor_set_to_cursor_cmd(&msg->cursor_set, msg_pos);
     if (!cmd) {
         return handle_msg_invalid(nullptr);
     }
-    cursor_channel->process_cmd(cmd);
+    cursor_channel->process_cmd(std::move(cmd));
 
     return true;
 }
@@ -478,12 +475,12 @@ StreamDevice::handle_msg_cursor_move()
     move->x = GINT32_FROM_LE(move->x);
     move->y = GINT32_FROM_LE(move->y);
 
-    auto cmd = g_new0(RedCursorCmd, 1);
+    auto cmd = red::make_shared<RedCursorCmd>();
     cmd->type = QXL_CURSOR_MOVE;
     cmd->u.position.x = move->x;
     cmd->u.position.y = move->y;
 
-    cursor_channel->process_cmd(cmd);
+    cursor_channel->process_cmd(std::move(cmd));
 
     return true;
 }

server/red-worker.cpp

@@ -90,16 +90,13 @@ struct RedWorker {
 
 static gboolean red_process_cursor_cmd(RedWorker *worker, const QXLCommandExt *ext)
 {
-    RedCursorCmd *cursor_cmd;
-
-    cursor_cmd = red_cursor_cmd_new(worker->qxl, &worker->mem_slots,
-                                    ext->group_id, ext->cmd.data);
-    if (cursor_cmd == nullptr) {
+    auto cursor_cmd = red_cursor_cmd_new(worker->qxl, &worker->mem_slots,
+                                         ext->group_id, ext->cmd.data);
+    if (!cursor_cmd) {
         return FALSE;
     }
 
-    worker->cursor_channel->process_cmd(cursor_cmd);
-    red_cursor_cmd_unref(cursor_cmd);
+    worker->cursor_channel->process_cmd(std::move(cursor_cmd));
 
     return TRUE;
 }

server/tests/Makefile.am

@@ -83,6 +83,7 @@ endif
 test_channel_SOURCES = test-channel.cpp
 test_stream_device_SOURCES = test-stream-device.cpp
 test_dispatcher_SOURCES = test-dispatcher.cpp
+test_qxl_parsing_SOURCES = test-qxl-parsing.cpp
 
 if !OS_WIN32
 check_PROGRAMS +=				\

server/tests/meson.build

@@ -44,7 +44,7 @@ tests = [
   ['test-stat', true],
   ['test-agent-msg-filter', true],
   ['test-loop', true],
-  ['test-qxl-parsing', true],
+  ['test-qxl-parsing', true, 'cpp'],
   ['test-leaks', true],
   ['test-vdagent', true],
   ['test-fail-on-null-core-interface', true],

server/tests/test-qxl-parsing.cpp

@@ -176,7 +176,6 @@ static void test_too_big_image(void)
 static void test_cursor_command(void)
 {
     RedMemSlotInfo mem_info;
-    RedCursorCmd *red_cursor_cmd;
     QXLCursorCmd cursor_cmd;
     QXLCursor *cursor;
 
@@ -194,9 +193,9 @@ static void test_cursor_command(void)
 
     cursor_cmd.u.set.shape = to_physical(cursor);
 
-    red_cursor_cmd = red_cursor_cmd_new(NULL, &mem_info, 0, to_physical(&cursor_cmd));
-    g_assert_nonnull(red_cursor_cmd);
-    red_cursor_cmd_unref(red_cursor_cmd);
+    auto red_cursor_cmd = red_cursor_cmd_new(NULL, &mem_info, 0, to_physical(&cursor_cmd));
+    g_assert(red_cursor_cmd);
+    red_cursor_cmd.reset();
     g_free(cursor);
     memslot_info_destroy(&mem_info);
 }
@@ -204,7 +203,6 @@ static void test_cursor_command(void)
 static void test_circular_empty_chunks(void)
 {
     RedMemSlotInfo mem_info;
-    RedCursorCmd *red_cursor_cmd;
     QXLCursorCmd cursor_cmd;
     QXLCursor *cursor;
     QXLDataChunk *chunks[2];
@@ -228,14 +226,14 @@ static void test_circular_empty_chunks(void)
 
     cursor_cmd.u.set.shape = to_physical(cursor);
 
-    red_cursor_cmd = red_cursor_cmd_new(NULL, &mem_info, 0, to_physical(&cursor_cmd));
-    if (red_cursor_cmd != NULL) {
+    auto red_cursor_cmd = red_cursor_cmd_new(NULL, &mem_info, 0, to_physical(&cursor_cmd));
+    if (red_cursor_cmd) {
         /* function does not return errors so there should be no data */
         g_assert_cmpuint(red_cursor_cmd->type, ==, QXL_CURSOR_SET);
         g_assert_cmpuint(red_cursor_cmd->u.set.position.x, ==, 0);
         g_assert_cmpuint(red_cursor_cmd->u.set.position.y, ==, 0);
         g_assert_cmpuint(red_cursor_cmd->u.set.shape.data_size, ==, 0);
-        red_cursor_cmd_unref(red_cursor_cmd);
+        red_cursor_cmd.reset();
     }
     g_test_assert_expected_messages();
 
@@ -247,7 +245,6 @@ static void test_circular_empty_chunks(void)
 static void test_circular_small_chunks(void)
 {
     RedMemSlotInfo mem_info;
-    RedCursorCmd *red_cursor_cmd;
     QXLCursorCmd cursor_cmd;
     QXLCursor *cursor;
     QXLDataChunk *chunks[2];
@@ -271,14 +268,14 @@ static void test_circular_small_chunks(void)
 
     cursor_cmd.u.set.shape = to_physical(cursor);
 
-    red_cursor_cmd = red_cursor_cmd_new(NULL, &mem_info, 0, to_physical(&cursor_cmd));
-    if (red_cursor_cmd != NULL) {
+    auto red_cursor_cmd = red_cursor_cmd_new(NULL, &mem_info, 0, to_physical(&cursor_cmd));
+    if (red_cursor_cmd) {
         /* function does not return errors so there should be no data */
         g_assert_cmpuint(red_cursor_cmd->type, ==, QXL_CURSOR_SET);
         g_assert_cmpuint(red_cursor_cmd->u.set.position.x, ==, 0);
         g_assert_cmpuint(red_cursor_cmd->u.set.position.y, ==, 0);
         g_assert_cmpuint(red_cursor_cmd->u.set.shape.data_size, ==, 0);
-        red_cursor_cmd_unref(red_cursor_cmd);
+        red_cursor_cmd.reset();
     }
     g_test_assert_expected_messages();