[Spice-devel] [PATCH 3/3] replay: Handle errors in record file
Jonathon Jongsma
jjongsma at redhat.com
Wed Jun 8 20:18:56 UTC 2016
On Wed, 2016-06-08 at 10:20 +0100, Frediano Ziglio wrote:
> Detect errors in record file. This can happen from a wrong version or
> corruption of files.
> Allocations are kept into a GList to be able to free in case some
> errors happened.
I think this allocation change makes it much more difficult to judge the patch.
I'd prefer to split this out, if it's necessary.
> Some macros are used to return error condition without many if.
> replay_t was converted to a pointer and REPLAY_EOF is NULL to have the
> same return for any errors.
> To check fscanf read all needed information a dummy "%n" is appended
> to any string and the value stored there is tested.
>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> server/red-replay-qxl.c | 228 ++++++++++++++++++++++++++++++++++-------------
> -
> 1 file changed, 162 insertions(+), 66 deletions(-)
>
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index b17c38b..5462cca 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -34,10 +34,9 @@
> #define QXLPHYSICAL_FROM_PTR(ptr) ((QXLPHYSICAL)(intptr_t)(ptr))
> #define QXLPHYSICAL_TO_PTR(phy) ((void*)(intptr_t)(phy))
>
> -typedef enum {
> - REPLAY_OK = 0,
> - REPLAY_EOF,
> -} replay_t;
> +typedef void *replay_t;
> +#define REPLAY_EOF NULL
> +#define REPLAY_OK ((replay_t)~(gsize)0)
>
> struct SpiceReplay {
> FILE *fd;
> @@ -49,6 +48,9 @@ struct SpiceReplay {
> GArray *id_map_inv; // replay id -> record id
> GArray *id_free; // free list
> int nsurfaces;
> + int end_pos;
> +
> + GList *allocated;
>
> pthread_mutex_t mutex;
> pthread_cond_t cond;
> @@ -66,12 +68,18 @@ static int replay_fread(SpiceReplay *replay, uint8_t *buf,
> size_t size)
> return fread(buf, size, 1, replay->fd);
> }
>
> +#define REPLAY_CHK(ret) do {\
> + if ((ret) == REPLAY_EOF) return NULL; \
> + }while(0)
> +
hmm, I don't really like this. It doesn't shorten things much, and it hides the
fact that there's a 'return' statement.
> __attribute__((format(scanf, 2, 3)))
> -static replay_t replay_fscanf(SpiceReplay *replay, const char *fmt, ...)
> +static replay_t replay_fscanf_check(SpiceReplay *replay, const char *fmt,
> ...)
> {
> va_list ap;
> int ret;
>
> + replay->end_pos = -1;
> +
> if (replay->eof) {
> return REPLAY_EOF;
> }
> @@ -82,11 +90,38 @@ static replay_t replay_fscanf(SpiceReplay *replay, const
> char *fmt, ...)
> va_start(ap, fmt);
> ret = vfscanf(replay->fd, fmt, ap);
> va_end(ap);
> - if (ret == EOF) {
> + if (ret == EOF || replay->end_pos < 0) {
I don't really like this either. It only works if the function is called in a
specific way (e.g. with a format string that includes '%n', etc). Of course, you
provide a macro (below) that forces it to be called in this way, but it still
feels like a bad idea to me...
Also, is it actually possible for replay->end_pos to be < 0 at this point? I
don't have much experience with using %n format strings, but it's supposed to
return the number of characters consumed from the input. So I would expect it to
set the value to 0 or greater?
> replay->eof = 1;
> }
> return replay->eof ? REPLAY_EOF : REPLAY_OK;
> }
> +#define replay_fscanf_check(r, fmt, ...) \
> + replay_fscanf_check(r, fmt "%n", ## __VA_ARGS__, &r->end_pos)
> +#define replay_fscanf(r, fmt, ...) \
> + REPLAY_CHK(replay_fscanf_check(r, fmt, ## __VA_ARGS__))
> +
> +static inline void add_allocated(SpiceReplay *replay, void *ptr)
> +{
> + replay->allocated = g_list_prepend(replay->allocated, ptr);
> + if (!replay->allocated) {
> + spice_error("error allocating memory");
> + abort();
> + }
As far as I know, g_list_prepend cannot return NULL. Glib generally aborts on
memory allocation failures.
> +}
> +
> +static inline void *replay_malloc(SpiceReplay *replay, size_t size)
> +{
> + void *p = spice_malloc(size);
> + add_allocated(replay, p);
> + return p;
> +}
> +
> +static inline void *replay_malloc0(SpiceReplay *replay, size_t size)
> +{
> + void *p = spice_malloc0(size);
> + add_allocated(replay, p);
> + return p;
> +}
>
> static uint32_t replay_id_get(SpiceReplay *replay, uint32_t id)
> {
> @@ -188,18 +223,18 @@ static void hexdump(uint8_t *hex, uint8_t bytes)
> static replay_t read_binary(SpiceReplay *replay, const char *prefix, size_t
> *size, uint8_t
> **buf, size_t base_size)
> {
> - char template[1024];
> + char file_prefix[1024];
> int with_zlib = -1;
> int zlib_size;
> - uint8_t *zlib_buffer;
> + uint8_t *zlib_buffer = NULL;
> z_stream strm;
>
> - snprintf(template, sizeof(template), "binary %%d %s %%ld:", prefix);
> - if (replay_fscanf(replay, template, &with_zlib, size) == REPLAY_EOF)
> + replay_fscanf(replay, "binary %d %1000s %ld:", &with_zlib, file_prefix,
> size);
> + if (strcmp(file_prefix, prefix) != 0)
This seems like an improvement, but it isn't strictly related to error-checking
per se.
> return REPLAY_EOF;
>
> if (*buf == NULL) {
> - *buf = spice_malloc(*size + base_size);
> + *buf = replay_malloc(replay, *size + base_size);
> }
> #if 0
> {
> @@ -211,9 +246,11 @@ static replay_t read_binary(SpiceReplay *replay, const
> char *prefix, size_t *siz
> spice_return_val_if_fail(with_zlib != -1, REPLAY_EOF);
> if (with_zlib) {
> int ret;
> + GList *elem;
>
> replay_fscanf(replay, "%d:", &zlib_size);
> - zlib_buffer = spice_malloc(zlib_size);
> + zlib_buffer = replay_malloc(replay, zlib_size);
> + elem = replay->allocated;
This part looks a bit "magic" unless you know that the implementation of
replay_malloc adds the buffer to relay->allocated.
> replay_fread(replay, zlib_buffer, zlib_size);
> strm.zalloc = Z_NULL;
> strm.zfree = Z_NULL;
> @@ -241,7 +278,9 @@ static replay_t read_binary(SpiceReplay *replay, const
> char *prefix, size_t *siz
> }
> }
> (void)inflateEnd(&strm);
> + replay->allocated = g_list_remove_link(replay->allocated, elem);
> free(zlib_buffer); // TODO - avoid repeat alloc/dealloc by keeping
> last
> + zlib_buffer = NULL;
> } else {
> replay_fread(replay, *buf + base_size, *size);
> }
> @@ -250,7 +289,7 @@ static replay_t read_binary(SpiceReplay *replay, const
> char *prefix, size_t *siz
> return REPLAY_OK;
> }
>
> -static size_t red_replay_data_chunks(SpiceReplay *replay, const char *prefix,
> +static ssize_t red_replay_data_chunks(SpiceReplay *replay, const char
> *prefix,
> uint8_t **mem, size_t base_size)
> {
> size_t data_size;
> @@ -301,34 +340,46 @@ static void red_replay_data_chunks_free(SpiceReplay
> *replay, void *data, size_t
> free(data);
> }
>
> -static void red_replay_point_ptr(SpiceReplay *replay, QXLPoint *qxl)
> +static replay_t red_replay_point_ptr(SpiceReplay *replay, QXLPoint *qxl)
> {
> replay_fscanf(replay, "point %d %d\n", &qxl->x, &qxl->y);
> + return REPLAY_OK;
> }
> +#define red_replay_point_ptr(r, q) \
> + REPLAY_CHK(red_replay_point_ptr(r, q))
>
> -static void red_replay_point16_ptr(SpiceReplay *replay, QXLPoint16 *qxl)
> +static replay_t red_replay_point16_ptr(SpiceReplay *replay, QXLPoint16 *qxl)
> {
> int x, y;
> replay_fscanf(replay, "point16 %d %d\n", &x, &y);
> qxl->x = x;
> qxl->y = y;
> + return REPLAY_OK;
> }
> +#define red_replay_point16_ptr(r, q) \
> + REPLAY_CHK(red_replay_point16_ptr(r, q))
>
> -static void red_replay_rect_ptr(SpiceReplay *replay, const char *prefix,
> QXLRect *qxl)
> +static replay_t red_replay_rect_ptr(SpiceReplay *replay, const char *prefix,
> QXLRect *qxl)
> {
> - char template[1024];
> + char file_prefix[1024];
>
> - snprintf(template, sizeof(template), "rect %s %%d %%d %%d %%d\n",
> prefix);
> - replay_fscanf(replay, template, &qxl->top, &qxl->left, &qxl->bottom,
> &qxl->right);
> + replay_fscanf(replay, "rect %1000s %d %d %d %d\n", file_prefix, &qxl-
> >top, &qxl->left, &qxl->bottom, &qxl->right);
> + if (strcmp(file_prefix, prefix) != 0)
> + return REPLAY_EOF;
> + return REPLAY_OK;
> }
> +#define red_replay_rect_ptr(r, p, q) \
> + REPLAY_CHK(red_replay_rect_ptr(r, p, q))
>
> static QXLPath *red_replay_path(SpiceReplay *replay)
> {
> QXLPath *qxl = NULL;
> - size_t data_size;
> + ssize_t data_size;
>
> data_size = red_replay_data_chunks(replay, "path", (uint8_t**)&qxl,
> sizeof(QXLPath));
> - qxl->data_size = data_size;
> + if (data_size >= 0) {
> + qxl->data_size = data_size;
> + }
> return qxl;
> }
>
> @@ -345,8 +396,8 @@ static QXLClipRects *red_replay_clip_rects(SpiceReplay
> *replay)
> int num_rects;
>
> replay_fscanf(replay, "num_rects %d\n", &num_rects);
> - red_replay_data_chunks(replay, "clip_rects", (uint8_t**)&qxl,
> sizeof(QXLClipRects));
> - qxl->num_rects = num_rects;
> + if (red_replay_data_chunks(replay, "clip_rects", (uint8_t**)&qxl,
> sizeof(QXLClipRects)) >= 0)
> + qxl->num_rects = num_rects;
> return qxl;
> }
>
> @@ -365,8 +416,9 @@ static uint8_t *red_replay_image_data_flat(SpiceReplay
> *replay, size_t *size)
>
> static QXLImage *red_replay_image(SpiceReplay *replay, uint32_t flags)
> {
> - QXLImage* qxl = NULL;
> - size_t bitmap_size, size;
> + QXLImage *qxl = NULL;
> + size_t bitmap_size;
> + ssize_t size;
> uint8_t qxl_flags;
> int temp;
> int has_palette;
> @@ -377,7 +429,7 @@ static QXLImage *red_replay_image(SpiceReplay *replay,
> uint32_t flags)
> return NULL;
> }
>
> - qxl = spice_new0(QXLImage, 1);
> + qxl = (QXLImage*)replay_malloc0(replay, sizeof(QXLImage));
> replay_fscanf(replay, "descriptor.id %"PRIu64"\n", &qxl->descriptor.id);
> replay_fscanf(replay, "descriptor.type %d\n", &temp); qxl-
> >descriptor.type = temp;
> replay_fscanf(replay, "descriptor.flags %d\n", &temp); qxl-
> >descriptor.flags = temp;
> @@ -398,7 +450,7 @@ static QXLImage *red_replay_image(SpiceReplay *replay,
> uint32_t flags)
> int i, num_ents;
>
> replay_fscanf(replay, "qp.num_ents %d\n", &num_ents);
> - qp = spice_malloc(sizeof(QXLPalette) + num_ents * sizeof(qp-
> >ents[0]));
> + qp = replay_malloc(replay, sizeof(QXLPalette) + num_ents *
> sizeof(qp->ents[0]));
> qp->num_ents = num_ents;
> qxl->bitmap.palette = QXLPHYSICAL_FROM_PTR(qp);
> replay_fscanf(replay, "unique %"PRIu64"\n", &qp->unique);
> @@ -413,7 +465,7 @@ static QXLImage *red_replay_image(SpiceReplay *replay,
> uint32_t flags)
> if (qxl_flags & QXL_BITMAP_DIRECT) {
> qxl->bitmap.data =
> QXLPHYSICAL_FROM_PTR(red_replay_image_data_flat(replay, &bitmap_size));
> } else {
> - size = red_replay_data_chunks(replay, "bitmap.data",
> (uint8_t**)&qxl->bitmap.data, 0);
> + ssize_t size = red_replay_data_chunks(replay, "bitmap.data",
> (uint8_t**)&qxl->bitmap.data, 0);
> if (size != bitmap_size) {
> spice_printerr("bad image, %zu != %zu", size, bitmap_size);
> return NULL;
> @@ -466,7 +518,7 @@ static void red_replay_image_free(SpiceReplay *replay,
> QXLPHYSICAL p, uint32_t f
> free(qxl);
> }
>
> -static void red_replay_brush_ptr(SpiceReplay *replay, QXLBrush *qxl, uint32_t
> flags)
> +static replay_t red_replay_brush_ptr(SpiceReplay *replay, QXLBrush *qxl,
> uint32_t flags)
> {
> replay_fscanf(replay, "type %d\n", &qxl->type);
> switch (qxl->type) {
> @@ -478,7 +530,10 @@ static void red_replay_brush_ptr(SpiceReplay *replay,
> QXLBrush *qxl, uint32_t fl
> red_replay_point_ptr(replay, &qxl->u.pattern.pos);
> break;
> }
> + return REPLAY_OK;
> }
> +#define red_replay_brush_ptr(r, q, f) \
> + REPLAY_CHK(red_replay_brush_ptr(r, q, f))
>
> static void red_replay_brush_free(SpiceReplay *replay, QXLBrush *qxl,
> uint32_t flags)
> {
> @@ -489,27 +544,31 @@ static void red_replay_brush_free(SpiceReplay *replay,
> QXLBrush *qxl, uint32_t f
> }
> }
>
> -static void red_replay_qmask_ptr(SpiceReplay *replay, QXLQMask *qxl, uint32_t
> flags)
> +static replay_t red_replay_qmask_ptr(SpiceReplay *replay, QXLQMask *qxl,
> uint32_t flags)
> {
> int temp;
>
> replay_fscanf(replay, "flags %d\n", &temp); qxl->flags = temp;
> red_replay_point_ptr(replay, &qxl->pos);
> qxl->bitmap = QXLPHYSICAL_FROM_PTR(red_replay_image(replay, flags));
> + return REPLAY_OK;
> }
> +#define red_replay_qmask_ptr(r, q, f) \
> + REPLAY_CHK(red_replay_qmask_ptr(r, q, f))
>
> static void red_replay_qmask_free(SpiceReplay *replay, QXLQMask *qxl,
> uint32_t flags)
> {
> red_replay_image_free(replay, qxl->bitmap, flags);
> }
>
> -static void red_replay_fill_ptr(SpiceReplay *replay, QXLFill *qxl, uint32_t
> flags)
> +static replay_t red_replay_fill_ptr(SpiceReplay *replay, QXLFill *qxl,
> uint32_t flags)
> {
> int temp;
>
> red_replay_brush_ptr(replay, &qxl->brush, flags);
> replay_fscanf(replay, "rop_descriptor %d\n", &temp); qxl->rop_descriptor
> = temp;
> red_replay_qmask_ptr(replay, &qxl->mask, flags);
> + return REPLAY_OK;
> }
>
> static void red_replay_fill_free(SpiceReplay *replay, QXLFill *qxl, uint32_t
> flags)
> @@ -518,7 +577,7 @@ static void red_replay_fill_free(SpiceReplay *replay,
> QXLFill *qxl, uint32_t fla
> red_replay_qmask_free(replay, &qxl->mask, flags);
> }
>
> -static void red_replay_opaque_ptr(SpiceReplay *replay, QXLOpaque *qxl,
> uint32_t flags)
> +static replay_t red_replay_opaque_ptr(SpiceReplay *replay, QXLOpaque *qxl,
> uint32_t flags)
> {
> int temp;
>
> @@ -528,6 +587,7 @@ static void red_replay_opaque_ptr(SpiceReplay *replay,
> QXLOpaque *qxl, uint32_t
> replay_fscanf(replay, "rop_descriptor %d\n", &temp); qxl->rop_descriptor
> = temp;
> replay_fscanf(replay, "scale_mode %d\n", &temp); qxl->scale_mode = temp;
> red_replay_qmask_ptr(replay, &qxl->mask, flags);
> + return REPLAY_OK;
> }
>
> static void red_replay_opaque_free(SpiceReplay *replay, QXLOpaque *qxl,
> uint32_t flags)
> @@ -537,7 +597,7 @@ static void red_replay_opaque_free(SpiceReplay *replay,
> QXLOpaque *qxl, uint32_t
> red_replay_qmask_free(replay, &qxl->mask, flags);
> }
>
> -static void red_replay_copy_ptr(SpiceReplay *replay, QXLCopy *qxl, uint32_t
> flags)
> +static replay_t red_replay_copy_ptr(SpiceReplay *replay, QXLCopy *qxl,
> uint32_t flags)
> {
> int temp;
>
> @@ -546,6 +606,7 @@ static void red_replay_copy_ptr(SpiceReplay *replay,
> QXLCopy *qxl, uint32_t flag
> replay_fscanf(replay, "rop_descriptor %d\n", &temp); qxl->rop_descriptor =
> temp;
> replay_fscanf(replay, "scale_mode %d\n", &temp); qxl->scale_mode = temp;
> red_replay_qmask_ptr(replay, &qxl->mask, flags);
> + return REPLAY_OK;
> }
>
> static void red_replay_copy_free(SpiceReplay *replay, QXLCopy *qxl, uint32_t
> flags)
> @@ -554,7 +615,7 @@ static void red_replay_copy_free(SpiceReplay *replay,
> QXLCopy *qxl, uint32_t fla
> red_replay_qmask_free(replay, &qxl->mask, flags);
> }
>
> -static void red_replay_blend_ptr(SpiceReplay *replay, QXLBlend *qxl, uint32_t
> flags)
> +static replay_t red_replay_blend_ptr(SpiceReplay *replay, QXLBlend *qxl,
> uint32_t flags)
> {
> int temp;
>
> @@ -563,6 +624,7 @@ static void red_replay_blend_ptr(SpiceReplay *replay,
> QXLBlend *qxl, uint32_t fl
> replay_fscanf(replay, "rop_descriptor %d\n", &temp); qxl->rop_descriptor =
> temp;
> replay_fscanf(replay, "scale_mode %d\n", &temp); qxl->scale_mode = temp;
> red_replay_qmask_ptr(replay, &qxl->mask, flags);
> + return REPLAY_OK;
> }
>
> static void red_replay_blend_free(SpiceReplay *replay, QXLBlend *qxl,
> uint32_t flags)
> @@ -571,12 +633,13 @@ static void red_replay_blend_free(SpiceReplay *replay,
> QXLBlend *qxl, uint32_t f
> red_replay_qmask_free(replay, &qxl->mask, flags);
> }
>
> -static void red_replay_transparent_ptr(SpiceReplay *replay, QXLTransparent
> *qxl, uint32_t flags)
> +static replay_t red_replay_transparent_ptr(SpiceReplay *replay,
> QXLTransparent *qxl, uint32_t flags)
> {
> qxl->src_bitmap = QXLPHYSICAL_FROM_PTR(red_replay_image(replay, flags));
> red_replay_rect_ptr(replay, "src_area", &qxl->src_area);
> replay_fscanf(replay, "src_color %d\n", &qxl->src_color);
> replay_fscanf(replay, "true_color %d\n", &qxl->true_color);
> + return REPLAY_OK;
> }
>
> static void red_replay_transparent_free(SpiceReplay *replay, QXLTransparent
> *qxl, uint32_t flags)
> @@ -584,7 +647,7 @@ static void red_replay_transparent_free(SpiceReplay
> *replay, QXLTransparent *qxl
> red_replay_image_free(replay, qxl->src_bitmap, flags);
> }
>
> -static void red_replay_alpha_blend_ptr(SpiceReplay *replay, QXLAlphaBlend
> *qxl, uint32_t flags)
> +static replay_t red_replay_alpha_blend_ptr(SpiceReplay *replay, QXLAlphaBlend
> *qxl, uint32_t flags)
> {
> int temp;
>
> @@ -592,6 +655,7 @@ static void red_replay_alpha_blend_ptr(SpiceReplay
> *replay, QXLAlphaBlend *qxl,
> replay_fscanf(replay, "alpha %d\n", &temp); qxl->alpha = temp;
> qxl->src_bitmap = QXLPHYSICAL_FROM_PTR(red_replay_image(replay, flags));
> red_replay_rect_ptr(replay, "src_area", &qxl->src_area);
> + return REPLAY_OK;
> }
>
> static void red_replay_alpha_blend_free(SpiceReplay *replay, QXLAlphaBlend
> *qxl, uint32_t flags)
> @@ -599,16 +663,17 @@ static void red_replay_alpha_blend_free(SpiceReplay
> *replay, QXLAlphaBlend *qxl,
> red_replay_image_free(replay, qxl->src_bitmap, flags);
> }
>
> -static void red_replay_alpha_blend_ptr_compat(SpiceReplay *replay,
> QXLCompatAlphaBlend *qxl, uint32_t flags)
> +static replay_t red_replay_alpha_blend_ptr_compat(SpiceReplay *replay,
> QXLCompatAlphaBlend *qxl, uint32_t flags)
> {
> int temp;
>
> replay_fscanf(replay, "alpha %d\n", &temp); qxl->alpha = temp;
> qxl->src_bitmap = QXLPHYSICAL_FROM_PTR(red_replay_image(replay, flags));
> red_replay_rect_ptr(replay, "src_area", &qxl->src_area);
> + return REPLAY_OK;
> }
>
> -static void red_replay_rop3_ptr(SpiceReplay *replay, QXLRop3 *qxl, uint32_t
> flags)
> +static replay_t red_replay_rop3_ptr(SpiceReplay *replay, QXLRop3 *qxl,
> uint32_t flags)
> {
> int temp;
>
> @@ -618,6 +683,7 @@ static void red_replay_rop3_ptr(SpiceReplay *replay,
> QXLRop3 *qxl, uint32_t flag
> replay_fscanf(replay, "rop3 %d\n", &temp); qxl->rop3 = temp;
> replay_fscanf(replay, "scale_mode %d\n", &temp); qxl->scale_mode = temp;
> red_replay_qmask_ptr(replay, &qxl->mask, flags);
> + return REPLAY_OK;
> }
>
> static void red_replay_rop3_free(SpiceReplay *replay, QXLRop3 *qxl, uint32_t
> flags)
> @@ -627,7 +693,7 @@ static void red_replay_rop3_free(SpiceReplay *replay,
> QXLRop3 *qxl, uint32_t fla
> red_replay_qmask_free(replay, &qxl->mask, flags);
> }
>
> -static void red_replay_stroke_ptr(SpiceReplay *replay, QXLStroke *qxl,
> uint32_t flags)
> +static replay_t red_replay_stroke_ptr(SpiceReplay *replay, QXLStroke *qxl,
> uint32_t flags)
> {
> int temp;
>
> @@ -642,6 +708,7 @@ static void red_replay_stroke_ptr(SpiceReplay *replay,
> QXLStroke *qxl, uint32_t
> red_replay_brush_ptr(replay, &qxl->brush, flags);
> replay_fscanf(replay, "fore_mode %d\n", &temp); qxl->fore_mode = temp;
> replay_fscanf(replay, "back_mode %d\n", &temp); qxl->back_mode = temp;
> + return REPLAY_OK;
> }
>
> static void red_replay_stroke_free(SpiceReplay *replay, QXLStroke *qxl,
> uint32_t flags)
> @@ -659,17 +726,19 @@ static QXLString *red_replay_string(SpiceReplay *replay)
> uint32_t data_size;
> uint16_t length;
> uint16_t flags;
> - size_t chunk_size;
> + ssize_t chunk_size;
> QXLString *qxl = NULL;
>
> replay_fscanf(replay, "data_size %d\n", &data_size);
> replay_fscanf(replay, "length %d\n", &temp); length = temp;
> replay_fscanf(replay, "flags %d\n", &temp); flags = temp;
> chunk_size = red_replay_data_chunks(replay, "string", (uint8_t**)&qxl,
> sizeof(QXLString));
> - qxl->data_size = data_size;
> - qxl->length = length;
> - qxl->flags = flags;
> - spice_assert(chunk_size == qxl->data_size);
> + if (chunk_size >= 0) {
> + qxl->data_size = data_size;
> + qxl->length = length;
> + qxl->flags = flags;
> + spice_assert(chunk_size == qxl->data_size);
> + }
> return qxl;
> }
>
> @@ -678,7 +747,7 @@ static void red_replay_string_free(SpiceReplay *replay,
> QXLString *qxl)
> red_replay_data_chunks_free(replay, qxl, sizeof(*qxl));
> }
>
> -static void red_replay_text_ptr(SpiceReplay *replay, QXLText *qxl, uint32_t
> flags)
> +static replay_t red_replay_text_ptr(SpiceReplay *replay, QXLText *qxl,
> uint32_t flags)
> {
> int temp;
>
> @@ -688,6 +757,7 @@ static void red_replay_text_ptr(SpiceReplay *replay,
> QXLText *qxl, uint32_t flag
> red_replay_brush_ptr(replay, &qxl->back_brush, flags);
> replay_fscanf(replay, "fore_mode %d\n", &temp); qxl->fore_mode = temp;
> replay_fscanf(replay, "back_mode %d\n", &temp); qxl->back_mode = temp;
> + return REPLAY_OK;
> }
>
> static void red_replay_text_free(SpiceReplay *replay, QXLText *qxl, uint32_t
> flags)
> @@ -697,9 +767,10 @@ static void red_replay_text_free(SpiceReplay *replay,
> QXLText *qxl, uint32_t fla
> red_replay_brush_free(replay, &qxl->back_brush, flags);
> }
>
> -static void red_replay_whiteness_ptr(SpiceReplay *replay, QXLWhiteness *qxl,
> uint32_t flags)
> +static replay_t red_replay_whiteness_ptr(SpiceReplay *replay, QXLWhiteness
> *qxl, uint32_t flags)
> {
> red_replay_qmask_ptr(replay, &qxl->mask, flags);
> + return REPLAY_OK;
> }
>
> static void red_replay_whiteness_free(SpiceReplay *replay, QXLWhiteness *qxl,
> uint32_t flags)
> @@ -707,9 +778,10 @@ static void red_replay_whiteness_free(SpiceReplay
> *replay, QXLWhiteness *qxl, ui
> red_replay_qmask_free(replay, &qxl->mask, flags);
> }
>
> -static void red_replay_blackness_ptr(SpiceReplay *replay, QXLBlackness *qxl,
> uint32_t flags)
> +static replay_t red_replay_blackness_ptr(SpiceReplay *replay, QXLBlackness
> *qxl, uint32_t flags)
> {
> red_replay_qmask_ptr(replay, &qxl->mask, flags);
> + return REPLAY_OK;
> }
>
> static void red_replay_blackness_free(SpiceReplay *replay, QXLBlackness *qxl,
> uint32_t flags)
> @@ -717,9 +789,10 @@ static void red_replay_blackness_free(SpiceReplay
> *replay, QXLBlackness *qxl, ui
> red_replay_qmask_free(replay, &qxl->mask, flags);
> }
>
> -static void red_replay_invers_ptr(SpiceReplay *replay, QXLInvers *qxl,
> uint32_t flags)
> +static replay_t red_replay_invers_ptr(SpiceReplay *replay, QXLInvers *qxl,
> uint32_t flags)
> {
> red_replay_qmask_ptr(replay, &qxl->mask, flags);
> + return REPLAY_OK;
> }
>
> static void red_replay_invers_free(SpiceReplay *replay, QXLInvers *qxl,
> uint32_t flags)
> @@ -727,7 +800,7 @@ static void red_replay_invers_free(SpiceReplay *replay,
> QXLInvers *qxl, uint32_t
> red_replay_qmask_free(replay, &qxl->mask, flags);
> }
>
> -static void red_replay_clip_ptr(SpiceReplay *replay, QXLClip *qxl)
> +static replay_t red_replay_clip_ptr(SpiceReplay *replay, QXLClip *qxl)
> {
> replay_fscanf(replay, "type %d\n", &qxl->type);
> switch (qxl->type) {
> @@ -735,6 +808,7 @@ static void red_replay_clip_ptr(SpiceReplay *replay,
> QXLClip *qxl)
> qxl->data = QXLPHYSICAL_FROM_PTR(red_replay_clip_rects(replay));
> break;
> }
> + return REPLAY_OK;
> }
>
> static void red_replay_clip_free(SpiceReplay *replay, QXLClip *qxl)
> @@ -757,7 +831,7 @@ static uint8_t *red_replay_transform(SpiceReplay *replay)
> return data;
> }
>
> -static void red_replay_composite_ptr(SpiceReplay *replay, QXLComposite *qxl,
> uint32_t flags)
> +static replay_t red_replay_composite_ptr(SpiceReplay *replay, QXLComposite
> *qxl, uint32_t flags)
> {
> int enabled;
>
> @@ -775,6 +849,7 @@ static void red_replay_composite_ptr(SpiceReplay *replay,
> QXLComposite *qxl, uin
>
> replay_fscanf(replay, "src_origin %" SCNi16 " %" SCNi16 "\n", &qxl-
> >src_origin.x, &qxl->src_origin.y);
> replay_fscanf(replay, "mask_origin %" SCNi16 " %" SCNi16 "\n", &qxl-
> >mask_origin.x, &qxl->mask_origin.y);
> + return REPLAY_OK;
> }
>
> static void red_replay_composite_free(SpiceReplay *replay, QXLComposite *qxl,
> uint32_t flags)
> @@ -788,7 +863,7 @@ static void red_replay_composite_free(SpiceReplay *replay,
> QXLComposite *qxl, ui
>
> static QXLDrawable *red_replay_native_drawable(SpiceReplay *replay, uint32_t
> flags)
> {
> - QXLDrawable *qxl = spice_malloc0(sizeof(QXLDrawable)); // TODO - this is
> too large usually
> + QXLDrawable *qxl = replay_malloc0(replay, sizeof(QXLDrawable)); // TODO -
> this is too large usually
> int i;
> int temp;
>
> @@ -857,7 +932,10 @@ static QXLDrawable
> *red_replay_native_drawable(SpiceReplay *replay, uint32_t fla
> spice_warn_if_reached();
> break;
> };
> - return qxl;
> + if (!replay->eof)
> + return qxl;
> +
> + return NULL;
> }
>
> static void red_replay_native_drawable_free(SpiceReplay *replay, QXLDrawable
> *qxl, uint32_t flags)
> @@ -919,7 +997,7 @@ static void red_replay_native_drawable_free(SpiceReplay
> *replay, QXLDrawable *qx
> static QXLCompatDrawable *red_replay_compat_drawable(SpiceReplay *replay,
> uint32_t flags)
> {
> int temp;
> - QXLCompatDrawable *qxl = spice_malloc0(sizeof(QXLCompatDrawable)); //
> TODO - too large usually
> + QXLCompatDrawable *qxl = replay_malloc0(replay,
> sizeof(QXLCompatDrawable)); // TODO - too large usually
>
> red_replay_rect_ptr(replay, "bbox", &qxl->bbox);
> red_replay_clip_ptr(replay, &qxl->clip);
> @@ -976,14 +1054,14 @@ static QXLCompatDrawable
> *red_replay_compat_drawable(SpiceReplay *replay, uint32
> spice_error("%s: unknown type %d", __FUNCTION__, qxl->type);
> break;
> };
> - return qxl;
> + if (!replay->eof)
> + return qxl;
> +
> + return NULL;
> }
>
> static QXLPHYSICAL red_replay_drawable(SpiceReplay *replay, uint32_t flags)
> {
> - if (replay->eof) {
> - return 0;
> - }
> replay_fscanf(replay, "drawable\n");
> if (flags & QXL_COMMAND_FLAG_COMPAT) {
> return QXLPHYSICAL_FROM_PTR(red_replay_compat_drawable(replay,
> flags));
> @@ -994,7 +1072,7 @@ static QXLPHYSICAL red_replay_drawable(SpiceReplay
> *replay, uint32_t flags)
>
> static QXLUpdateCmd *red_replay_update_cmd(SpiceReplay *replay)
> {
> - QXLUpdateCmd *qxl = spice_malloc0(sizeof(QXLUpdateCmd));
> + QXLUpdateCmd *qxl = replay_malloc0(replay, sizeof(QXLUpdateCmd));
>
> replay_fscanf(replay, "update\n");
> red_replay_rect_ptr(replay, "area", &qxl->area);
> @@ -1039,7 +1117,7 @@ static QXLSurfaceCmd *red_replay_surface_cmd(SpiceReplay
> *replay)
> spice_printerr("mismatch %zu != %zu", size, read_size);
> }
> } else {
> - qxl->u.surface_create.data =
> QXLPHYSICAL_FROM_PTR(spice_malloc(size));
> + qxl->u.surface_create.data =
> QXLPHYSICAL_FROM_PTR(replay_malloc(replay, size));
> }
> qxl->surface_id = replay_id_new(replay, qxl->surface_id);
> break;
> @@ -1123,7 +1201,7 @@ static void red_replay_cursor_cmd_free(SpiceReplay
> *replay, QXLCursorCmd *qxl)
> free(qxl);
> }
>
> -static void replay_handle_create_primary(QXLWorker *worker, SpiceReplay
> *replay)
> +static replay_t replay_handle_create_primary(QXLWorker *worker, SpiceReplay
> *replay)
> {
> QXLDevSurfaceCreate surface = { 0, };
> size_t size;
> @@ -1145,6 +1223,7 @@ static void replay_handle_create_primary(QXLWorker
> *worker, SpiceReplay *replay)
> surface.group_id = 0;
> surface.mem = QXLPHYSICAL_FROM_PTR(mem);
> worker->create_primary_surface(worker, 0, &surface);
> + return REPLAY_OK;
> }
>
> static void replay_handle_dev_input(QXLWorker *worker, SpiceReplay *replay,
> @@ -1185,18 +1264,16 @@ static void replay_handle_dev_input(QXLWorker *worker,
> SpiceReplay *replay,
> SPICE_GNUC_VISIBLE QXLCommandExt* spice_replay_next_cmd(SpiceReplay *replay,
> QXLWorker *worker)
> {
> - QXLCommandExt* cmd;
> + QXLCommandExt* cmd = NULL;
> uint64_t timestamp;
> int type;
> int what = -1;
> int counter;
>
> while (what != 0) {
> - replay_fscanf(replay, "event %d %d %d %"PRIu64"\n", &counter,
> - &what, &type, ×tamp);
> - if (replay->eof) {
> - return NULL;
> - }
> + if (replay_fscanf_check(replay, "event %d %d %d %"PRIu64"\n",
> &counter,
> + &what, &type, ×tamp) == REPLAY_EOF)
> + goto eof;
> if (what == 1) {
> replay_handle_dev_input(worker, replay, type);
> }
> @@ -1224,6 +1301,9 @@ SPICE_GNUC_VISIBLE QXLCommandExt*
> spice_replay_next_cmd(SpiceReplay *replay,
> break;
> }
>
> + if (replay->eof)
> + goto eof;
> +
> QXLReleaseInfo *info;
> switch (cmd->cmd.type) {
> case QXL_CMD_DRAW:
> @@ -1234,9 +1314,23 @@ SPICE_GNUC_VISIBLE QXLCommandExt*
> spice_replay_next_cmd(SpiceReplay *replay,
> info->id = (uintptr_t)cmd;
> }
>
> + if (replay->allocated) {
> + g_list_free(replay->allocated);
> + replay->allocated = NULL;
> + }
> +
I don't quite understand why you free the list without freeing the contents?
> replay->counter++;
>
> return cmd;
> +
> +eof:
> + if (replay->allocated) {
> + g_list_free_full(replay->allocated, free);
> + replay->allocated = NULL;
> + }
> + if (cmd)
> + g_free(cmd);
> + return NULL;
> }
>
> SPICE_GNUC_VISIBLE void spice_replay_free_cmd(SpiceReplay *replay,
> QXLCommandExt *cmd)
> @@ -1305,6 +1399,7 @@ SpiceReplay *spice_replay_new(FILE *file, int nsurfaces)
> replay->id_map_inv = g_array_new(FALSE, FALSE, sizeof(uint32_t));
> replay->id_free = g_array_new(FALSE, FALSE, sizeof(uint32_t));
> replay->nsurfaces = nsurfaces;
> + replay->allocated = g_list_alloc();
This is a bit unusual. Generally an empty GList is represented by NULL.
>
> /* reserve id 0 */
> replay_id_new(replay, 0);
> @@ -1316,6 +1411,7 @@ SPICE_GNUC_VISIBLE void spice_replay_free(SpiceReplay
> *replay)
> {
> spice_return_if_fail(replay != NULL);
>
> + g_list_free_full(replay->allocated, free);
> pthread_mutex_destroy(&replay->mutex);
> pthread_cond_destroy(&replay->cond);
> g_array_free(replay->id_map, TRUE);
Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
More information about the Spice-devel
mailing list