[Spice-devel] [RFC] server/red_parse_qxl: introduce visitor for cursor
Marc-André Lureau
marcandre.lureau at gmail.com
Thu Nov 15 12:02:39 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.
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 = 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.
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 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?
cheers
--
Marc-André Lureau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20121115/ed64356f/attachment.html>
More information about the Spice-devel
mailing list