[Spice-devel] [RFC] server/red_parse_qxl: introduce visitor for cursor

Thu Nov 15 12:49:33 PST 2012

> Hi
> 
> 
> 
> 
> On Wed, Nov 14, 2012 at 9:36 PM, Alon Levy < alevy at redhat.com >
> wrote:
> 
> 
> 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.

The first is about a year ago, I need to dig up the link. Will do in follow up. It was working and everything, I had some logs from it, included a replay utility, some screwed up ad hoc file format without any index, but worked. But the code was deemed unmaintainable due to it being produced by some sed scripts plus alot of hand wringing from red_parse_qxl.c I got red_record_qxl.c and another playback utility.

> 
> 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...

It's used of course to free the data allocated when we use the copy visitor. Do you mean it should be in a separate file 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 = 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);
> 
> 
> 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.

Yeah, Uri said the same, of course I agree that the names are bad, and splitting probably too, so yeah, I can make it so. This being an RFC I didn't bother.

> 
> 
> 
> 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
> Spice-devel at lists.freedesktop.org
> 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?

oh, I could do the logger first, probably a good idea. I was actually working on doing the rest of the to-visitor conversion first.

Not yet anything except this first patch, I'll post when I have it in a better way. The chunk stuff is giving me headaches (it doesn't appear in cursor, surface, message, only in drawable - which is 80% of the file anyway).

> 
> cheers
> 
> --
> Marc-André Lureau
> 