[Spice-devel] [spice-server v4 03/11] qxl: Fix guest resources release in red_put_drawable()

[Christophe Fergeau]

At the moment, we'll unconditionally release the guest QXL resources in
red_put_drawable() even if red_get_drawable() failed and did not
initialize drawable->release_info_ext properly.
This commit only sets RedDrawable::qxl once the guest resource have been
successfully retrieved, and only free the guest QXL resources when
RedDrawable::qxl is set.
---
server/red-parse-qxl.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 03a3a655a..15413af91 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1008,7 +1008,7 @@ static void red_put_clip(SpiceClip *red)
}
}

-static bool red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
+static bool red_get_native_drawable(QXLInstance *qxl_instance, RedMemSlotInfo *slots, int group_id,
RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
{
QXLDrawable *qxl;
@@ -1018,6 +1018,7 @@ static bool red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
if (qxl == NULL) {
return false;
}
+ red->qxl = qxl_instance;
red->release_info_ext.info = &qxl->release_info;
red->release_info_ext.group_id = group_id;

@@ -1088,7 +1089,7 @@ static bool red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
return true;
}

-static bool red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
+static bool red_get_compat_drawable(QXLInstance *qxl_instance, RedMemSlotInfo *slots, int group_id,
RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
{
QXLCompatDrawable *qxl;
@@ -1097,6 +1098,7 @@ static bool red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
if (qxl == NULL) {
return false;
}
+ red->qxl = qxl_instance;
red->release_info_ext.info = &qxl->release_info;
red->release_info_ext.group_id = group_id;

@@ -1170,15 +1172,15 @@ static bool red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
return true;
}

-static bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
+static bool red_get_drawable(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id,
RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
{
bool ret;

if (flags & QXL_COMMAND_FLAG_COMPAT) {
- ret = red_get_compat_drawable(slots, group_id, red, addr, flags);
+ ret = red_get_compat_drawable(qxl, slots, group_id, red, addr, flags);
} else {
- ret = red_get_native_drawable(slots, group_id, red, addr, flags);
+ ret = red_get_native_drawable(qxl, slots, group_id, red, addr, flags);
}
return ret;
}
@@ -1230,6 +1232,9 @@ static void red_put_drawable(RedDrawable *red)
red_put_whiteness(&red->u.whiteness);
break;
}
+ if (red->qxl != NULL) {
+ red_qxl_release_resource(red->qxl, red->release_info_ext);
+ }
}

bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
@@ -1481,7 +1486,6 @@ void red_drawable_unref(RedDrawable *red_drawable)
if (--red_drawable->refs) {
return;
}
- red_qxl_release_resource(red_drawable->qxl, red_drawable->release_info_ext);
red_put_drawable(red_drawable);
g_free(red_drawable);
}
@@ -1493,9 +1497,8 @@ RedDrawable *red_drawable_new(QXLInstance *qxl, RedMemSlotInfo *slots,
RedDrawable *red = g_new0(RedDrawable, 1);

red->refs = 1;
- red->qxl = qxl;

- if (!red_get_drawable(slots, group_id, red, addr, flags)) {
+ if (!red_get_drawable(qxl, slots, group_id, red, addr, flags)) {
red_drawable_unref(red);
return NULL;
}

[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);

[Christophe Fergeau]

---
server/red-parse-qxl.c | 11 +++++++----
server/red-parse-qxl.h | 3 ++-
server/red-worker.c | 3 +--
3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index e88e75096..1608caff2 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1237,8 +1237,9 @@ static void red_put_drawable(RedDrawable *red)
}
}

-bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
- RedUpdateCmd *red, QXLPHYSICAL addr)
+bool red_get_update_cmd(QXLInstance *qxl_instance, RedMemSlotInfo *slots,
+ int group_id, RedUpdateCmd *red,
+ QXLPHYSICAL addr)
{
QXLUpdateCmd *qxl;

@@ -1246,10 +1247,10 @@ bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
if (qxl == NULL) {
return false;
}
+ red->qxl = qxl_instance;
red->release_info_ext.info = &qxl->release_info;
red->release_info_ext.group_id = group_id;

-
red_get_rect_ptr(&red->area, &qxl->area);
red->update_id = qxl->update_id;
red->surface_id = qxl->surface_id;
@@ -1258,7 +1259,9 @@ bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,

void red_put_update_cmd(RedUpdateCmd *red)
{
- /* nothing yet */
+ if (red->qxl != NULL) {
+ red_qxl_release_resource(red->qxl, red->release_info_ext);
+ }
}

static bool red_get_message(QXLInstance *qxl_instance, RedMemSlotInfo *slots, int group_id,
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index fad144071..f9013161d 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -60,6 +60,7 @@ typedef struct RedDrawable {
} RedDrawable;

typedef struct RedUpdateCmd {
+ QXLInstance *qxl;
QXLReleaseInfoExt release_info_ext;
SpiceRect area;
uint32_t update_id;
@@ -119,7 +120,7 @@ RedDrawable *red_drawable_new(QXLInstance *qxl, RedMemSlotInfo *slots,
RedDrawable *red_drawable_ref(RedDrawable *drawable);
void red_drawable_unref(RedDrawable *red_drawable);

-bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
+bool red_get_update_cmd(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id,
RedUpdateCmd *red, QXLPHYSICAL addr);
void red_put_update_cmd(RedUpdateCmd *red);

diff --git a/server/red-worker.c b/server/red-worker.c
index 70d596dd6..7c9076c46 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -215,7 +215,7 @@ static int red_process_display(RedWorker *worker, int *ring_is_empty)
case QXL_CMD_UPDATE: {
RedUpdateCmd update;

- if (!red_get_update_cmd(&worker->mem_slots, ext_cmd.group_id,
+ if (!red_get_update_cmd(worker->qxl, &worker->mem_slots, ext_cmd.group_id,
&update, ext_cmd.cmd.data)) {
break;
}
@@ -225,7 +225,6 @@ static int red_process_display(RedWorker *worker, int *ring_is_empty)
display_channel_draw(worker->display_channel, &update.area, update.surface_id);
red_qxl_notify_update(worker->qxl, update.update_id);
}
- red_qxl_release_resource(worker->qxl, update.release_info_ext);
red_put_update_cmd(&update);
break;
}

[Christophe Fergeau]

---
server/display-channel.c | 3 ---
server/red-parse-qxl.c | 12 ++++++++----
server/red-parse-qxl.h | 1 +
3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/server/display-channel.c b/server/display-channel.c
index 91ef72215..e68ed10f8 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -286,7 +286,6 @@ static void stop_streams(DisplayChannel *display)
void display_channel_surface_unref(DisplayChannel *display, uint32_t surface_id)
{
RedSurface *surface = &display->priv->surfaces[surface_id];
- QXLInstance *qxl = display->priv->qxl;
DisplayChannelClient *dcc;

if (--surface->refs != 0) {
@@ -301,12 +300,10 @@ void display_channel_surface_unref(DisplayChannel *display, uint32_t surface_id)

surface->context.canvas->ops->destroy(surface->context.canvas);
if (surface->create_cmd != NULL) {
- red_qxl_release_resource(qxl, surface->create_cmd->release_info_ext);
red_surface_cmd_unref(surface->create_cmd);
surface->create_cmd = NULL;
}
if (surface->destroy_cmd != NULL) {
- red_qxl_release_resource(qxl, surface->destroy_cmd->release_info_ext);
red_surface_cmd_unref(surface->destroy_cmd);
surface->destroy_cmd = NULL;
}
diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 736e66d97..95c4c643f 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1411,8 +1411,9 @@ bool red_validate_surface(uint32_t width, uint32_t height,
return true;
}

-static bool red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
+static bool red_get_surface_cmd(QXLInstance *qxl_instance, RedMemSlotInfo *slots, int group_id,
RedSurfaceCmd *red, QXLPHYSICAL addr)
+
{
QXLSurfaceCmd *qxl;
uint64_t size;
@@ -1421,6 +1422,7 @@ static bool red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
if (qxl == NULL) {
return false;
}
+ red->qxl = qxl_instance;
red->release_info_ext.info = &qxl->release_info;
red->release_info_ext.group_id = group_id;

@@ -1453,7 +1455,9 @@ static bool red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,

static void red_put_surface_cmd(RedSurfaceCmd *red)
{
- /* nothing yet */
+ if (red->qxl) {
+ red_qxl_release_resource(red->qxl, red->release_info_ext);
+ }
}

RedSurfaceCmd *red_surface_cmd_new(QXLInstance *qxl_instance, RedMemSlotInfo *slots,
@@ -1465,8 +1469,8 @@ RedSurfaceCmd *red_surface_cmd_new(QXLInstance *qxl_instance, RedMemSlotInfo *sl

cmd->refs = 1;

- if (!red_get_surface_cmd(slots, group_id, cmd, addr)) {
- g_free(cmd);
+ if (!red_get_surface_cmd(qxl_instance, slots, group_id, cmd, addr)) {
+ red_surface_cmd_unref(cmd);
return NULL;
}

diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index 37e4ffb21..43ace663b 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -85,6 +85,7 @@ typedef struct RedSurfaceCreate {
} RedSurfaceCreate;

typedef struct RedSurfaceCmd {
+ QXLInstance *qxl;
QXLReleaseInfoExt release_info_ext;
int refs;
uint32_t surface_id;

[Christophe Fergeau]

Currently, RedUpdateCmd are allocated on the stack, and then
initialized/uninitialized with red_{get,put}_update_cmd
This makes the API inconsistent with what is being done for RedDrawable,
RedCursor and RedMessage. QXLUpdateCmd are not occurring very often,
we can dynamically allocate them instead, and get a consistent API.
---
server/red-parse-qxl.c | 38 ++++++++++++++++++++++++++++++++++----
server/red-parse-qxl.h | 7 ++++---
server/red-worker.c | 14 +++++++-------
3 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 1608caff2..00ec8b9c9 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1237,9 +1237,8 @@ static void red_put_drawable(RedDrawable *red)
}
}

-bool red_get_update_cmd(QXLInstance *qxl_instance, RedMemSlotInfo *slots,
- int group_id, RedUpdateCmd *red,
- QXLPHYSICAL addr)
+static bool red_get_update_cmd(QXLInstance *qxl_instance, RedMemSlotInfo *slots, int group_id,
+ RedUpdateCmd *red, QXLPHYSICAL addr)
{
QXLUpdateCmd *qxl;

@@ -1257,13 +1256,44 @@ bool red_get_update_cmd(QXLInstance *qxl_instance, RedMemSlotInfo *slots,
return true;
}

-void red_put_update_cmd(RedUpdateCmd *red)
+static void red_put_update_cmd(RedUpdateCmd *red)
{
if (red->qxl != NULL) {
red_qxl_release_resource(red->qxl, red->release_info_ext);
}
}

+RedUpdateCmd *red_update_cmd_new(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id, QXLPHYSICAL addr)
+{
+ RedUpdateCmd *red;
+
+ red = g_new0(RedUpdateCmd, 1);
+
+ red->refs = 1;
+
+ if (!red_get_update_cmd(qxl, slots, group_id, red, addr)) {
+ red_update_cmd_unref(red);
+ return NULL;
+ }
+
+ return red;
+}
+
+RedUpdateCmd *red_update_cmd_ref(RedUpdateCmd *red)
+{
+ red->refs++;
+ return red;
+}
+
+void red_update_cmd_unref(RedUpdateCmd *red)
+{
+ if (--red->refs) {
+ return;
+ }
+ red_put_update_cmd(red);
+ g_free(red);
+}
+
static bool red_get_message(QXLInstance *qxl_instance, RedMemSlotInfo *slots, int group_id,
RedMessage *red, QXLPHYSICAL addr)
{
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index f9013161d..d969b3004 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -62,6 +62,7 @@ typedef struct RedDrawable {
typedef struct RedUpdateCmd {
QXLInstance *qxl;
QXLReleaseInfoExt release_info_ext;
+ int refs;
SpiceRect area;
uint32_t update_id;
uint32_t surface_id;
@@ -120,9 +121,9 @@ RedDrawable *red_drawable_new(QXLInstance *qxl, RedMemSlotInfo *slots,
RedDrawable *red_drawable_ref(RedDrawable *drawable);
void red_drawable_unref(RedDrawable *red_drawable);

-bool red_get_update_cmd(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id,
- RedUpdateCmd *red, QXLPHYSICAL addr);
-void red_put_update_cmd(RedUpdateCmd *red);
+RedUpdateCmd *red_update_cmd_new(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id, QXLPHYSICAL addr);
+RedUpdateCmd *red_update_cmd_ref(RedUpdateCmd *red);
+void red_update_cmd_unref(RedUpdateCmd *red);

RedMessage *red_message_new(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id, QXLPHYSICAL addr);
RedMessage *red_message_ref(RedMessage *red);
diff --git a/server/red-worker.c b/server/red-worker.c
index 7c9076c46..7734fc04c 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -213,19 +213,19 @@ static int red_process_display(RedWorker *worker, int *ring_is_empty)
break;
}
case QXL_CMD_UPDATE: {
- RedUpdateCmd update;
+ RedUpdateCmd *update;

- if (!red_get_update_cmd(worker->qxl, &worker->mem_slots, ext_cmd.group_id,
- &update, ext_cmd.cmd.data)) {
+ update = red_update_cmd_new(worker->qxl, &worker->mem_slots, ext_cmd.group_id, ext_cmd.cmd.data);
+ if (update == NULL) {
break;
}
- if (!display_channel_validate_surface(worker->display_channel, update.surface_id)) {
+ if (!display_channel_validate_surface(worker->display_channel, update->surface_id)) {
spice_warning("Invalid surface in QXL_CMD_UPDATE");
} else {
- display_channel_draw(worker->display_channel, &update.area, update.surface_id);
- red_qxl_notify_update(worker->qxl, update.update_id);
+ display_channel_draw(worker->display_channel, &update->area, update->surface_id);
+ red_qxl_notify_update(worker->qxl, update->update_id);
}
- red_put_update_cmd(&update);
+ red_update_cmd_unref(update);
break;
}
case QXL_CMD_MESSAGE: {

[Christophe Fergeau]

RedDrawable really is a RedDrawCmd which is parsed by red-parse-qxl.h
Moreover, red_drawable_ref() is already defined inline in
red-parse-qxl.h, and red_drawable_unref() is declared there too even if
its code is still in red-worker.c
This commit moves them close to the other functions creating/unref'ing
QXL commands parsed by red-parse-qxl.h.
---
server/red-parse-qxl.c | 26 ++++++++++++++++++++++++++
server/red-parse-qxl.h | 12 ++++--------
server/red-worker.c | 20 --------------------
3 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 86abe3ca4..e1f547b7a 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1469,3 +1469,29 @@ void red_put_cursor_cmd(RedCursorCmd *red)
red_qxl_release_resource(red->qxl, red->release_info_ext);
}
}
+
+RedDrawable *red_drawable_ref(RedDrawable *drawable)
+{
+ drawable->refs++;
+ return drawable;
+}
+
+void red_drawable_unref(RedDrawable *red_drawable)
+{
+ if (--red_drawable->refs) {
+ return;
+ }
+ red_qxl_release_resource(red_drawable->qxl, red_drawable->release_info_ext);
+ red_put_drawable(red_drawable);
+ g_free(red_drawable);
+}
+
+RedDrawable *red_drawable_new(QXLInstance *qxl)
+{
+ RedDrawable * red = g_new0(RedDrawable, 1);
+
+ red->refs = 1;
+ red->qxl = qxl;
+
+ return red;
+}
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index a33f36adb..3b815a860 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -59,14 +59,6 @@ typedef struct RedDrawable {
} u;
} RedDrawable;

-static inline RedDrawable *red_drawable_ref(RedDrawable *drawable)
-{
- drawable->refs++;
- return drawable;
-}
-
-void red_drawable_unref(RedDrawable *red_drawable);
-
typedef struct RedUpdateCmd {
QXLReleaseInfoExt release_info_ext;
SpiceRect area;
@@ -118,6 +110,10 @@ typedef struct RedCursorCmd {

void red_get_rect_ptr(SpiceRect *red, const QXLRect *qxl);

+RedDrawable *red_drawable_new(QXLInstance *qxl);
+RedDrawable *red_drawable_ref(RedDrawable *drawable);
+void red_drawable_unref(RedDrawable *red_drawable);
+
bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
RedDrawable *red, QXLPHYSICAL addr, uint32_t flags);
void red_put_drawable(RedDrawable *red);
diff --git a/server/red-worker.c b/server/red-worker.c
index ccab9d960..d9ae792af 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -92,16 +92,6 @@ struct RedWorker {
GMainLoop *loop;
};

-void red_drawable_unref(RedDrawable *red_drawable)
-{
- if (--red_drawable->refs) {
- return;
- }
- red_qxl_release_resource(red_drawable->qxl, red_drawable->release_info_ext);
- red_put_drawable(red_drawable);
- g_free(red_drawable);
-}
-
static gboolean red_process_cursor_cmd(RedWorker *worker, const QXLCommandExt *ext)
{
RedCursorCmd *cursor_cmd;
@@ -158,16 +148,6 @@ static int red_process_cursor(RedWorker *worker, int *ring_is_empty)
return n;
}

-static RedDrawable *red_drawable_new(QXLInstance *qxl)
-{
- RedDrawable * red = g_new0(RedDrawable, 1);
-
- red->refs = 1;
- red->qxl = qxl;
-
- return red;
-}
-
static gboolean red_process_surface_cmd(RedWorker *worker, QXLCommandExt *ext, gboolean loadvm)
{
RedSurfaceCmd surface_cmd;

[Christophe Fergeau]

Rather than needing to call red_drawable_new() and then initialize it
with red_get_drawable(), we can improve slightly red_drawable new so
that red_drawable_{new,ref,unref} is all which is used by code out of
red-parse-qxl.c.
---
server/red-parse-qxl.c | 17 ++++++++++++-----
server/red-parse-qxl.h | 8 +++-----
server/red-worker.c | 11 ++++++-----
3 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index e1f547b7a..03a3a655a 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1170,8 +1170,8 @@ static bool red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
return true;
}

-bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
- RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
+static bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
+ RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
{
bool ret;

@@ -1183,7 +1183,7 @@ bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
return ret;
}

-void red_put_drawable(RedDrawable *red)
+static void red_put_drawable(RedDrawable *red)
{
red_put_clip(&red->clip);
if (red->self_bitmap_image) {
@@ -1486,12 +1486,19 @@ void red_drawable_unref(RedDrawable *red_drawable)
g_free(red_drawable);
}

-RedDrawable *red_drawable_new(QXLInstance *qxl)
+RedDrawable *red_drawable_new(QXLInstance *qxl, RedMemSlotInfo *slots,
+ int group_id, QXLPHYSICAL addr,
+ uint32_t flags)
{
- RedDrawable * red = g_new0(RedDrawable, 1);
+ RedDrawable *red = g_new0(RedDrawable, 1);

red->refs = 1;
red->qxl = qxl;

+ if (!red_get_drawable(slots, group_id, red, addr, flags)) {
+ red_drawable_unref(red);
+ return NULL;
+ }
+
return red;
}
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index 3b815a860..cbb880192 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -110,14 +110,12 @@ typedef struct RedCursorCmd {

void red_get_rect_ptr(SpiceRect *red, const QXLRect *qxl);

-RedDrawable *red_drawable_new(QXLInstance *qxl);
+RedDrawable *red_drawable_new(QXLInstance *qxl, RedMemSlotInfo *slots,
+ int group_id, QXLPHYSICAL addr,
+ uint32_t flags);
RedDrawable *red_drawable_ref(RedDrawable *drawable);
void red_drawable_unref(RedDrawable *red_drawable);

-bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
- RedDrawable *red, QXLPHYSICAL addr, uint32_t flags);
-void red_put_drawable(RedDrawable *red);
-
bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
RedUpdateCmd *red, QXLPHYSICAL addr);
void red_put_update_cmd(RedUpdateCmd *red);
diff --git a/server/red-worker.c b/server/red-worker.c
index d9ae792af..9168553db 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -198,15 +198,16 @@ static int red_process_display(RedWorker *worker, int *ring_is_empty)
worker->display_poll_tries = 0;
switch (ext_cmd.cmd.type) {
case QXL_CMD_DRAW: {
- RedDrawable *red_drawable = red_drawable_new(worker->qxl); // returns with 1 ref
+ RedDrawable *red_drawable;
+ red_drawable = red_drawable_new(worker->qxl, &worker->mem_slots,
+ ext_cmd.group_id, ext_cmd.cmd.data,
+ ext_cmd.flags); // returns with 1 ref

- if (red_get_drawable(&worker->mem_slots, ext_cmd.group_id,
- red_drawable, ext_cmd.cmd.data, ext_cmd.flags)) {
+ if (red_drawable != NULL) {
display_channel_process_draw(worker->display_channel, red_drawable,
worker->process_display_generation);
+ red_drawable_unref(red_drawable);
}
- // release the red_drawable
- red_drawable_unref(red_drawable);
break;
}
case QXL_CMD_UPDATE: {

[Christophe Fergeau]

Currently, the cursor channel is allocating RedCursorCmd instances itself, and then
calling into red-parse-qxl.h to initialize it, and doing something
similar when releasing the data. This commit moves this common code to
red-parse-qxl.[ch]
The ref/unref are not strictly needed, red_cursor_cmd_free() would
currently be enough, but this makes the API consistent with
red_drawable_{new,ref,unref}.
---
server/cursor-channel.c | 8 ++-----
server/red-parse-qxl.c | 40 ++++++++++++++++++++++++++++++---
server/red-parse-qxl.h | 8 ++++---
server/red-worker.c | 12 +++++-----
server/tests/test-qxl-parsing.c | 37 ++++++++++++++++--------------
5 files changed, 70 insertions(+), 35 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index ada1d62a7..bb85f0edd 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -62,20 +62,16 @@ static RedCursorPipeItem *cursor_pipe_item_new(RedCursorCmd *cmd)

red_pipe_item_init_full(&item->base, RED_PIPE_ITEM_TYPE_CURSOR,
cursor_pipe_item_free);
- item->red_cursor = cmd;
+ item->red_cursor = red_cursor_cmd_ref(cmd);

return item;
}

static void cursor_pipe_item_free(RedPipeItem *base)
{
- RedCursorCmd *cursor_cmd;
RedCursorPipeItem *pipe_item = SPICE_UPCAST(RedCursorPipeItem, base);

- cursor_cmd = pipe_item->red_cursor;
- red_put_cursor_cmd(cursor_cmd);
- g_free(cursor_cmd);
-
+ red_cursor_cmd_unref(pipe_item->red_cursor);
g_free(pipe_item);
}

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 15413af91..7e6f5ce9d 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1434,8 +1434,9 @@ static void red_put_cursor(SpiceCursor *red)
g_free(red->data);
}

-bool red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
- RedCursorCmd *red, QXLPHYSICAL addr)
+static bool red_get_cursor_cmd(QXLInstance *qxl_instance, RedMemSlotInfo *slots,
+ int group_id, RedCursorCmd *red,
+ QXLPHYSICAL addr)
{
QXLCursorCmd *qxl;

@@ -1443,6 +1444,7 @@ bool red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
if (qxl == NULL) {
return false;
}
+ red->qxl = qxl_instance;
red->release_info_ext.info = &qxl->release_info;
red->release_info_ext.group_id = group_id;

@@ -1463,7 +1465,24 @@ bool red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
return true;
}

-void red_put_cursor_cmd(RedCursorCmd *red)
+RedCursorCmd *red_cursor_cmd_new(QXLInstance *qxl, RedMemSlotInfo *slots,
+ int group_id, QXLPHYSICAL addr)
+{
+ RedCursorCmd *cmd;
+
+ 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 NULL;
+ }
+
+ return cmd;
+}
+
+static void red_put_cursor_cmd(RedCursorCmd *red)
{
switch (red->type) {
case QXL_CURSOR_SET:
@@ -1475,6 +1494,21 @@ void red_put_cursor_cmd(RedCursorCmd *red)
}
}

+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);
+}
+
RedDrawable *red_drawable_ref(RedDrawable *drawable)
{
drawable->refs++;
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index cbb880192..cb1c1b9fa 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -93,6 +93,7 @@ typedef struct RedSurfaceCmd {
typedef struct RedCursorCmd {
QXLInstance *qxl;
QXLReleaseInfoExt release_info_ext;
+ int refs;
uint8_t type;
union {
struct {
@@ -131,8 +132,9 @@ bool red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
RedSurfaceCmd *red, QXLPHYSICAL addr);
void red_put_surface_cmd(RedSurfaceCmd *red);

-bool red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
- RedCursorCmd *red, QXLPHYSICAL addr);
-void red_put_cursor_cmd(RedCursorCmd *red);
+RedCursorCmd *red_cursor_cmd_new(QXLInstance *qxl, RedMemSlotInfo *slots,
+ int group_id, QXLPHYSICAL addr);
+RedCursorCmd *red_cursor_cmd_ref(RedCursorCmd *cmd);
+void red_cursor_cmd_unref(RedCursorCmd *cmd);

#endif /* RED_PARSE_QXL_H_ */
diff --git a/server/red-worker.c b/server/red-worker.c
index 9168553db..58fe32d32 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -96,13 +96,15 @@ static gboolean red_process_cursor_cmd(RedWorker *worker, const QXLCommandExt *e
{
RedCursorCmd *cursor_cmd;

- cursor_cmd = g_new0(RedCursorCmd, 1);
- if (!red_get_cursor_cmd(&worker->mem_slots, ext->group_id, cursor_cmd, ext->cmd.data)) {
- g_free(cursor_cmd);
+ cursor_cmd = red_cursor_cmd_new(worker->qxl, &worker->mem_slots,
+ ext->group_id, ext->cmd.data);
+ if (cursor_cmd == NULL) {
return FALSE;
}
- cursor_cmd->qxl = worker->qxl;
+
cursor_channel_process_cmd(worker->cursor_channel, cursor_cmd);
+ red_cursor_cmd_unref(cursor_cmd);
+
return TRUE;
}

@@ -956,10 +958,8 @@ static bool loadvm_command(RedWorker *worker, QXLCommandExt *ext)
switch (ext->cmd.type) {
case QXL_CMD_CURSOR:
return red_process_cursor_cmd(worker, ext);
-
case QXL_CMD_SURFACE:
return red_process_surface_cmd(worker, ext, TRUE);
-
default:
spice_warning("unhandled loadvm command type (%d)", ext->cmd.type);
}
diff --git a/server/tests/test-qxl-parsing.c b/server/tests/test-qxl-parsing.c
index 47139a48c..e757bdfac 100644
--- a/server/tests/test-qxl-parsing.c
+++ b/server/tests/test-qxl-parsing.c
@@ -149,7 +149,7 @@ static void test_too_big_image(void)
static void test_cursor_command(void)
{
RedMemSlotInfo mem_info;
- RedCursorCmd red_cursor_cmd;
+ RedCursorCmd *red_cursor_cmd;
QXLCursorCmd cursor_cmd;
QXLCursor *cursor;

@@ -167,8 +167,9 @@ static void test_cursor_command(void)

cursor_cmd.u.set.shape = to_physical(cursor);

- g_assert_true(red_get_cursor_cmd(&mem_info, 0, &red_cursor_cmd, to_physical(&cursor_cmd)));
- g_free(red_cursor_cmd.u.set.shape.data);
+ red_cursor_cmd = red_cursor_cmd_new(NULL, &mem_info, 0, to_physical(&cursor_cmd));
+ g_assert_true(red_cursor_cmd != NULL);
+ red_cursor_cmd_unref(red_cursor_cmd);
g_free(cursor);
memslot_info_destroy(&mem_info);
}
@@ -176,7 +177,7 @@ static void test_cursor_command(void)
static void test_circular_empty_chunks(void)
{
RedMemSlotInfo mem_info;
- RedCursorCmd red_cursor_cmd;
+ RedCursorCmd *red_cursor_cmd;
QXLCursorCmd cursor_cmd;
QXLCursor *cursor;
QXLDataChunk *chunks[2];
@@ -200,13 +201,14 @@ static void test_circular_empty_chunks(void)

cursor_cmd.u.set.shape = to_physical(cursor);

- memset(&red_cursor_cmd, 0xaa, sizeof(red_cursor_cmd));
- if (red_get_cursor_cmd(&mem_info, 0, &red_cursor_cmd, to_physical(&cursor_cmd))) {
+ red_cursor_cmd = red_cursor_cmd_new(NULL, &mem_info, 0, to_physical(&cursor_cmd));
+ if (red_cursor_cmd != NULL) {
/* 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);
+ 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);
}
g_test_assert_expected_messages();

@@ -218,7 +220,7 @@ static void test_circular_empty_chunks(void)
static void test_circular_small_chunks(void)
{
RedMemSlotInfo mem_info;
- RedCursorCmd red_cursor_cmd;
+ RedCursorCmd *red_cursor_cmd;
QXLCursorCmd cursor_cmd;
QXLCursor *cursor;
QXLDataChunk *chunks[2];
@@ -242,13 +244,14 @@ static void test_circular_small_chunks(void)

cursor_cmd.u.set.shape = to_physical(cursor);

- memset(&red_cursor_cmd, 0xaa, sizeof(red_cursor_cmd));
- if (red_get_cursor_cmd(&mem_info, 0, &red_cursor_cmd, to_physical(&cursor_cmd))) {
+ red_cursor_cmd = red_cursor_cmd_new(NULL, &mem_info, 0, to_physical(&cursor_cmd));
+ if (red_cursor_cmd != NULL) {
/* 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);
+ 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);
}
g_test_assert_expected_messages();

[Christophe Fergeau]

Now that we have a refcounted RedSurfaceCmd, we can store the command
itself in DisplayChannel rather than copying QXLReleaseInfoExt. This
will let us move the release of the QXL guest resources in red-parse-qxl
in the next commit.
---
server/display-channel-private.h | 7 ++++++-
server/display-channel.c | 22 +++++++++++++---------
server/display-channel.h | 2 +-
3 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/server/display-channel-private.h b/server/display-channel-private.h
index 27f0a0191..58179531d 100644
--- a/server/display-channel-private.h
+++ b/server/display-channel-private.h
@@ -52,7 +52,12 @@ typedef struct RedSurface {
QRegion draw_dirty_region;

//fix me - better handling here
- QXLReleaseInfoExt create, destroy;
+ /* '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;
+ /* QEMU expects the guest data for the command to be valid as long as the
+ * surface is valid */
+ RedSurfaceCmd *destroy_cmd;
} RedSurface;

typedef struct MonitorsConfig {
diff --git a/server/display-channel.c b/server/display-channel.c
index 118f795f0..91ef72215 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -300,11 +300,15 @@ 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.info) {
- red_qxl_release_resource(qxl, surface->create);
+ if (surface->create_cmd != NULL) {
+ red_qxl_release_resource(qxl, surface->create_cmd->release_info_ext);
+ red_surface_cmd_unref(surface->create_cmd);
+ surface->create_cmd = NULL;
}
- if (surface->destroy.info) {
- red_qxl_release_resource(qxl, surface->destroy);
+ if (surface->destroy_cmd != NULL) {
+ red_qxl_release_resource(qxl, surface->destroy_cmd->release_info_ext);
+ red_surface_cmd_unref(surface->destroy_cmd);
+ surface->destroy_cmd = NULL;
}

region_destroy(&surface->draw_dirty_region);
@@ -2161,8 +2165,8 @@ void display_channel_create_surface(DisplayChannel *display, uint32_t surface_id
}
memset(data, 0, height*abs(stride));
}
- surface->create.info = NULL;
- surface->destroy.info = NULL;
+ g_warn_if_fail(surface->create_cmd == NULL);
+ g_warn_if_fail(surface->destroy_cmd == NULL);
ring_init(&surface->current);
ring_init(&surface->current_list);
ring_init(&surface->depend_on_me);
@@ -2306,7 +2310,7 @@ display_channel_constructed(GObject *object)
}

void display_channel_process_surface_cmd(DisplayChannel *display,
- const RedSurfaceCmd *surface_cmd,
+ RedSurfaceCmd *surface_cmd,
int loadvm)
{
uint32_t surface_id;
@@ -2342,7 +2346,7 @@ void display_channel_process_surface_cmd(DisplayChannel *display,
reloaded_surface,
// reloaded surfaces will be sent on demand
!reloaded_surface);
- surface->create = surface_cmd->release_info_ext;
+ surface->create_cmd = red_surface_cmd_ref(surface_cmd);
break;
}
case QXL_SURFACE_CMD_DESTROY:
@@ -2350,7 +2354,7 @@ void display_channel_process_surface_cmd(DisplayChannel *display,
spice_warning("avoiding destroying a surface twice");
break;
}
- surface->destroy = surface_cmd->release_info_ext;
+ surface->destroy_cmd = red_surface_cmd_ref(surface_cmd);
display_channel_destroy_surface(display, surface_id);
break;
default:
diff --git a/server/display-channel.h b/server/display-channel.h
index 455e224f5..5fcf7035a 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -147,7 +147,7 @@ void display_channel_process_draw (DisplayCha
RedDrawable *red_drawable,
uint32_t process_commands_generation);
void display_channel_process_surface_cmd (DisplayChannel *display,
- const RedSurfaceCmd *surface_cmd,
+ RedSurfaceCmd *surface_cmd,
int loadvm);
void display_channel_update_compression (DisplayChannel *display,
DisplayChannelClient *dcc);

[Christophe Fergeau]

---
server/red-parse-qxl.c | 7 +++++--
server/red-parse-qxl.h | 3 ++-
server/red-worker.c | 3 +--
3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 7e6f5ce9d..4be53d3e5 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1261,7 +1261,7 @@ void red_put_update_cmd(RedUpdateCmd *red)
/* nothing yet */
}

-bool red_get_message(RedMemSlotInfo *slots, int group_id,
+bool red_get_message(QXLInstance *qxl_instance, RedMemSlotInfo *slots, int group_id,
RedMessage *red, QXLPHYSICAL addr)
{
QXLMessage *qxl;
@@ -1279,6 +1279,7 @@ bool red_get_message(RedMemSlotInfo *slots, int group_id,
if (qxl == NULL) {
return false;
}
+ red->qxl = qxl_instance;
red->release_info_ext.info = &qxl->release_info;
red->release_info_ext.group_id = group_id;
red->data = qxl->data;
@@ -1295,7 +1296,9 @@ bool red_get_message(RedMemSlotInfo *slots, int group_id,

void red_put_message(RedMessage *red)
{
- /* nothing yet */
+ if (red->qxl != NULL) {
+ red_qxl_release_resource(red->qxl, red->release_info_ext);
+ }
}

static unsigned int surface_format_to_bpp(uint32_t format)
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index cb1c1b9fa..ecf7b1577 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -67,6 +67,7 @@ typedef struct RedUpdateCmd {
} RedUpdateCmd;

typedef struct RedMessage {
+ QXLInstance *qxl;
QXLReleaseInfoExt release_info_ext;
int len;
uint8_t *data;
@@ -121,7 +122,7 @@ bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
RedUpdateCmd *red, QXLPHYSICAL addr);
void red_put_update_cmd(RedUpdateCmd *red);

-bool red_get_message(RedMemSlotInfo *slots, int group_id,
+bool red_get_message(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id,
RedMessage *red, QXLPHYSICAL addr);
void red_put_message(RedMessage *red);

diff --git a/server/red-worker.c b/server/red-worker.c
index 58fe32d32..aa54dcd85 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -232,14 +232,13 @@ static int red_process_display(RedWorker *worker, int *ring_is_empty)
case QXL_CMD_MESSAGE: {
RedMessage message;

- if (!red_get_message(&worker->mem_slots, ext_cmd.group_id,
+ if (!red_get_message(worker->qxl, &worker->mem_slots, ext_cmd.group_id,
&message, ext_cmd.cmd.data)) {
break;
}
#ifdef DEBUG
spice_warning("MESSAGE: %.*s", message.len, message.data);
#endif
- red_qxl_release_resource(worker->qxl, message.release_info_ext);
red_put_message(&message);
break;
}

[Christophe Fergeau]

Currently, RedMessage are allocated on the stack, and then
initialized/uninitialized with red_{get,put}_message
This makes the API inconsistent with what is being done for RedDrawable
and RedCursor. Since QXLMessage is just a (mostly unused/unsecure) debugging tool,
we can dynamically allocate it instead, and get a consistent API.
---
server/red-parse-qxl.c | 37 ++++++++++++++++++++++++++++++++++---
server/red-parse-qxl.h | 7 ++++---
server/red-worker.c | 8 ++++----
3 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 4be53d3e5..e88e75096 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1261,8 +1261,8 @@ void red_put_update_cmd(RedUpdateCmd *red)
/* nothing yet */
}

-bool red_get_message(QXLInstance *qxl_instance, RedMemSlotInfo *slots, int group_id,
- RedMessage *red, QXLPHYSICAL addr)
+static bool red_get_message(QXLInstance *qxl_instance, RedMemSlotInfo *slots, int group_id,
+ RedMessage *red, QXLPHYSICAL addr)
{
QXLMessage *qxl;
int memslot_id;
@@ -1294,13 +1294,44 @@ bool red_get_message(QXLInstance *qxl_instance, RedMemSlotInfo *slots, int group
return true;
}

-void red_put_message(RedMessage *red)
+static void red_put_message(RedMessage *red)
{
if (red->qxl != NULL) {
red_qxl_release_resource(red->qxl, red->release_info_ext);
}
}

+RedMessage *red_message_new(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id, QXLPHYSICAL addr)
+{
+ RedMessage *red;
+
+ red = g_new0(RedMessage, 1);
+
+ red->refs = 1;
+
+ if (!red_get_message(qxl, slots, group_id, red, addr)) {
+ red_message_unref(red);
+ return NULL;
+ }
+
+ return red;
+}
+
+RedMessage *red_message_ref(RedMessage *red)
+{
+ red->refs++;
+ return red;
+}
+
+void red_message_unref(RedMessage *red)
+{
+ if (--red->refs) {
+ return;
+ }
+ red_put_message(red);
+ g_free(red);
+}
+
static unsigned int surface_format_to_bpp(uint32_t format)
{
switch (format) {
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index ecf7b1577..fad144071 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -69,6 +69,7 @@ typedef struct RedUpdateCmd {
typedef struct RedMessage {
QXLInstance *qxl;
QXLReleaseInfoExt release_info_ext;
+ int refs;
int len;
uint8_t *data;
} RedMessage;
@@ -122,9 +123,9 @@ bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
RedUpdateCmd *red, QXLPHYSICAL addr);
void red_put_update_cmd(RedUpdateCmd *red);

-bool red_get_message(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id,
- RedMessage *red, QXLPHYSICAL addr);
-void red_put_message(RedMessage *red);
+RedMessage *red_message_new(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id, QXLPHYSICAL addr);
+RedMessage *red_message_ref(RedMessage *red);
+void red_message_unref(RedMessage *red);

bool red_validate_surface(uint32_t width, uint32_t height,
int32_t stride, uint32_t format);
diff --git a/server/red-worker.c b/server/red-worker.c
index aa54dcd85..70d596dd6 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -230,16 +230,16 @@ static int red_process_display(RedWorker *worker, int *ring_is_empty)
break;
}
case QXL_CMD_MESSAGE: {
- RedMessage message;
+ RedMessage *message;

- if (!red_get_message(worker->qxl, &worker->mem_slots, ext_cmd.group_id,
- &message, ext_cmd.cmd.data)) {
+ message = red_message_new(worker->qxl, &worker->mem_slots, ext_cmd.group_id, ext_cmd.cmd.data);
+ if (message == NULL) {
break;
}
#ifdef DEBUG
spice_warning("MESSAGE: %.*s", message.len, message.data);
#endif
- red_put_message(&message);
+ red_message_unref(message);
break;
}
case QXL_CMD_SURFACE:

[Frediano Ziglio]

Hey,
Currently, after parsing a QXL command through red-parse-qxl, the code which
got the command has to tell red-parse-qxl when it no longer needs the
command, but also to remember to release the command QXL resources
itself. This series moves this 'release resource' logic to
red-parse-qxl. This also changes all the parsed QXL commands to use
_new/_ref/_unref in order to have a consistent API.
- rebased on top of master
- dropped the last 3 patches which were RFC and did not get any
feedback. I'll resend once this series has moved forward.
- switched all QXL command parsing to use _new/_ref/_unref
- added new patches doing this for RedSurfaceCmd
- added patches introducing RedQXLGuestResources and using QXLCommandExt in
red-parse-qxl.h interface
- moved renaming patch to the end, and made it much more extensive
- added a new patch unifying identical methods
- reworked 'qxl: Fix guest resources release in red_put_drawable()' so that
it's similar to
the cursor changes
Christophe

Acked the series with minor changes (spaces, line breaks and some function
orders).

Frediano