Hi <div class="gmail_extra"> <div class="gmail_quote">On Wed, Nov 14, 2012 at 9:36 PM, Alon Levy <<a href="mailto:alevy@redhat.com" target="_blank">alevy@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. </blockquote><div> 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 </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> --- Does this look doable? it's the beginning of a second try at recording qxl traffic for debugging and development. </blockquote><div> Yeah, we really want that. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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; </blockquote><div> 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. </div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + void (*visit_cursor_cmd)(void *opaque, QXLCursorCmd *qxl); </blockquote><div> And opaque could be "self" instead :) </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + 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); +} </blockquote><div>this function could go bellow, since it isn't used by CursorCmdVisitor. Actually it could even be removed perhaps... </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +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 = data; + } else { + red->data = spice_malloc(size); + 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 = red_linearize_chunk(&chunks, size, &free_data); red_put_data_chunks(&chunks); - if (free_data) { - red->data = data; - } else { - red->data = spice_malloc(size); - 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); </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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(&copy_cursor_visitor, addr); +} + void red_put_cursor_cmd(RedCursorCmd *red) { switch (red->type) { -- 1.8.0 _______________________________________________ Spice-devel mailing list <a href="mailto:Spice-devel@lists.freedesktop.org">Spice-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/spice-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/spice-devel</a> </blockquote></div> 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 </div>