From 8669c933fff70ce2c75498c4ef04f6bb7bd20d36 Mon Sep 17 00:00:00 2001 From: Christophe Fergeau Date: Thu, 29 Nov 2018 13:50:11 +0100 Subject: [PATCH] qxl: Add red_surface_cmd_{new, ref, unref} helpers 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. Signed-off-by: Christophe Fergeau Acked-by: Frediano Ziglio --- server/red-parse-qxl.c | 38 ++++++++++++++++++++++++++++++--- server/red-parse-qxl.h | 8 ++++--- server/red-worker.c | 17 ++++++++------- server/tests/test-qxl-parsing.c | 16 ++++++++------ 4 files changed, 59 insertions(+), 20 deletions(-) diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c index efdd06e2..c1812af1 100644 --- a/server/red-parse-qxl.c +++ b/server/red-parse-qxl.c @@ -1444,8 +1444,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; @@ -1484,11 +1484,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 6fb7b3c5..7abd3847 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; @@ -134,9 +135,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 04ad4c9d..f016943a 100644 --- a/server/red-worker.c +++ b/server/red-worker.c @@ -152,16 +152,17 @@ 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 2314d768..324f7fdc 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);