On Wed, Nov 14, 2012 at 9:36 PM, Alon Levy <alevy@xxxxxxxxxx> wrote:
I missed the first version somehow.
So a bit of context could be helpful :) Here we are changing the code that read cursor cmds from qxl memory into RedCursorCmd *red
Yeah, we really want that.
Instead of opaque pointer we could have each visitor implementation derive from that base visitor, but this isn't incompatible anyway, a matter of taste perhaps.
And opaque could be "self" instead :)
Is there a valid reason why set_1 and set_2 are splitted? Looks to me that it could be the same function eventually? And merge back visit_cursor_cmd_set in it's caller.
This is a second attempt at recording commands received from the qxl
device. The visitor allows more code to be shared between the device to
spice struct verifier and copier and the logger.
I missed the first version somehow.
So a bit of context could be helpful :) Here we are changing the code that read cursor cmds from qxl memory into RedCursorCmd *red
---
Does this look doable? it's the beginning of a second try at recording qxl
traffic for debugging and development.
Yeah, we really want that.
server/red_parse_qxl.c | 142 ++++++++++++++++++++++++++++++++++++-------------
1 file changed, 104 insertions(+), 38 deletions(-)
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index 82463e1..298a334 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -1251,8 +1251,88 @@ void red_put_surface_cmd(RedSurfaceCmd *red)
/* nothing yet */
}
-static int red_get_cursor(RedMemSlotInfo *slots, int group_id,
- SpiceCursor *red, QXLPHYSICAL addr)
+typedef struct CursorCmdVisitor {
+ RedMemSlotInfo *slots;
+ int group_id;
+ void *opaque;
Instead of opaque pointer we could have each visitor implementation derive from that base visitor, but this isn't incompatible anyway, a matter of taste perhaps.
+ void (*visit_cursor_cmd)(void *opaque, QXLCursorCmd *qxl);
And opaque could be "self" instead :)
+ void (*visit_cursor_set_1)(void *opaque, QXLPoint16 *position, uint8_t visible);
+ void (*visit_cursor_set_2)(void *opaque, QXLCursorHeader *header,
+ uint32_t data_size, bool free_data, size_t size,
+ uint8_t *data);
+ void (*visit_cursor_move)(void *opaque, QXLPoint16 *position);
+ void (*visit_cursor_trail)(void *opaque, uint16_t length, uint16_t frequency);
+} CursorCmdVisitor;
+
+static void red_put_cursor(SpiceCursor *red)
+{
+ free(red->data);
+}
this function could go bellow, since it isn't used by CursorCmdVisitor. Actually it could even be removed perhaps...
+static void copy_visit_cursor_cmd(void *opaque, QXLCursorCmd *qxl)
+{
+ RedCursorCmd *cmd = opaque;
+
+ cmd->release_info = &qxl->release_info;
+ cmd->type = qxl->type;
+}
+
+void copy_visit_cursor_set_1(void *opaque, QXLPoint16 *position, uint8_t visible)
+{
+ RedCursorCmd *red = opaque;
+
+ red_get_point16_ptr(&red->u.set.position, position);
+ red->u.set.visible = visible;
+}
+
+static void copy_visit_cursor_set_2(void *opaque, QXLCursorHeader *header,
+ uint32_t data_size, bool free_data, size_t size,
+ uint8_t *data)
+{
+ RedCursorCmd *cmd = opaque;
+ SpiceCursor *red = &cmd->u.set.shape;
+
+ red->header.unique = header->unique;
+ red->header.type = header->type;
+ red->header.width = header->width;
+ red->header.height = header->height;
+ red->header.hot_spot_x = header->hot_spot_x;
+ red->header.hot_spot_y = header->hot_spot_y;
+
+ red->flags = 0;
+ red->data_size = data_size;
+ if (free_data) {
+ red->data = ""> + } else {
+ red->data = ""> + memcpy(red->data, data, size);
+ }
+}
+
+void copy_visit_cursor_move(void *opaque, QXLPoint16 *position)
+{
+ RedCursorCmd *red = opaque;
+
+ red_get_point16_ptr(&red->u.position, position);
+}
+
+void copy_visit_cursor_trail(void *opaque, uint16_t length, uint16_t frequency)
+{
+ RedCursorCmd *red = opaque;
+
+ red->u.trail.length = length;
+ red->u.trail.frequency = frequency;
+}
+
+CursorCmdVisitor copy_cursor_visitor = {
+ .visit_cursor_cmd = copy_visit_cursor_cmd,
+ .visit_cursor_set_1 = copy_visit_cursor_set_1,
+ .visit_cursor_set_2 = copy_visit_cursor_set_2,
+ .visit_cursor_move = copy_visit_cursor_move,
+ .visit_cursor_trail = copy_visit_cursor_trail,
+};
+
+static int visit_cursor_cmd_set(CursorCmdVisitor *v, QXLPHYSICAL addr)
{
QXLCursor *qxl;
RedDataChunk chunks;
@@ -1261,69 +1341,55 @@ static int red_get_cursor(RedMemSlotInfo *slots, int group_id,
bool free_data;
int error;
- qxl = (QXLCursor *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
+ qxl = (QXLCursor *)get_virt(v->slots, addr, sizeof(*qxl), v->group_id, &error);
if (error) {
return 1;
}
- red->header.unique = qxl->header.unique;
- red->header.type = qxl->header.type;
- red->header.width = qxl->header.width;
- red->header.height = qxl->header.height;
- red->header.hot_spot_x = qxl->header.hot_spot_x;
- red->header.hot_spot_y = qxl->header.hot_spot_y;
-
- red->flags = 0;
- red->data_size = qxl->data_size;
- size = red_get_data_chunks_ptr(slots, group_id,
- get_memslot_id(slots, addr),
+ size = red_get_data_chunks_ptr(v->slots, v->group_id,
+ get_memslot_id(v->slots, addr),
&chunks, &qxl->chunk);
data = "" size, &free_data);
red_put_data_chunks(&chunks);
- if (free_data) {
- red->data = ""> - } else {
- red->data = ""> - memcpy(red->data, data, size);
- }
+ v->visit_cursor_set_2(v->opaque, &qxl->header, qxl->data_size,
+ free_data, size, data);
return 0;
}
-static void red_put_cursor(SpiceCursor *red)
-{
- free(red->data);
-}
-
-int red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
- RedCursorCmd *red, QXLPHYSICAL addr)
+int visit_cursor_cmd(CursorCmdVisitor *v, QXLPHYSICAL addr)
{
QXLCursorCmd *qxl;
int error;
- qxl = (QXLCursorCmd *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
+ qxl = (QXLCursorCmd *)get_virt(v->slots, addr, sizeof(*qxl), v->group_id, &error);
if (error) {
return error;
}
- red->release_info = &qxl->release_info;
-
- red->type = qxl->type;
- switch (red->type) {
+ v->visit_cursor_cmd(v->opaque, qxl);
+ switch (qxl->type) {
case QXL_CURSOR_SET:
- red_get_point16_ptr(&red->u.set.position, &qxl->u.set.position);
- red->u.set.visible = qxl->u.set.visible;
- error = red_get_cursor(slots, group_id, &red->u.set.shape, qxl->u.set.shape);
+ v->visit_cursor_set_1(v->opaque, &qxl->u.set.position, qxl->u.set.visible);
+ error = visit_cursor_cmd_set(v, qxl->u.set.shape);
Is there a valid reason why set_1 and set_2 are splitted? Looks to me that it could be the same function eventually? And merge back visit_cursor_cmd_set in it's caller.
break;
case QXL_CURSOR_MOVE:
- red_get_point16_ptr(&red->u.position, &qxl->u.position);
+ v->visit_cursor_move(v->opaque, &qxl->u.position);
break;
case QXL_CURSOR_TRAIL:
- red->u.trail.length = qxl->u.trail.length;
- red->u.trail.frequency = qxl->u.trail.frequency;
+ v->visit_cursor_trail(v->opaque, qxl->u.trail.length, qxl->u.trail.frequency);
break;
}
return error;
}
+int red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
+ RedCursorCmd *red, QXLPHYSICAL addr)
+{
+ copy_cursor_visitor.slots = slots;
+ copy_cursor_visitor.group_id = group_id;
+ copy_cursor_visitor.opaque = red;
+ return visit_cursor_cmd(©_cursor_visitor, addr);
+}
+
void red_put_cursor_cmd(RedCursorCmd *red)
{
switch (red->type) {
--
1.8.0
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Otherwise, looks "fine". I suppose you want to take similar approach for other commands? It would be nice to see some bits of the curosr logger visitor as an example of where this is going. Do you have a wip branch?
cheers
--
Marc-André Lureau
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel