>
> 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@xxxxxxxxxx>
> > ---
> > 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.
>
I expanded this macro and checked the result. Was really ugly!
But looking at the result I realized that accumulating the error in the eof field
I could avoid many checks. The code require a bit more attention as it's not
automatic that the code won't use uninitialized data.
> > __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...
>
Removed lot of macros.
> 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?
>
Let's assume the format string is "value %d\n%n", if the string to parse is
- "". An EOF is returned ;
- "value garbage". A 0 is returned, "%n" is not handled so the value
stays as set before XXscanf (in this case -1);
- "value 123". A 1 is returned, "%n" is not handled;
- "value 123 other". Same as above
- "value 123\nother". A 2 is returned, "%n" is handled and set to position
(in this case 10).
So adding "%n" and check the value is handled make sure parsing is done
correctly.
Assuming the format string is "value %d\n" (without the "%n" trick) and
strings are "value 123 garbage" and "value 123\n" in both cases XXscanf
returns 1 but the first value is not valid as it does not contain the "\n".
The "%n" trick also avoid some problem with XXscanf returning different
return values if values are not stored due to portability issue (but this
should not be our problem).
> > 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.
>
Removed the add_allocated function.
> > +}
> > +
> > +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.
>
This is needed as the replay_fscanf (which is a macro) appends a constant
string to an expected constant string.
> > 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.
>
Yes... I don't see much of a problem. I could use a g_list_remove..
just performance craziness on my side I assume.
> > 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?
>
Basically the list is used to track the allocations. In case of failure we
free list and content to avoid leaks, in case of success we want to return
allocated data (the content) but we free the list as not needed anymore
(and to avoid the old data to be freed next time).
I put a comment on these lines.
> > 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.
>
Changed
> >
> > /* 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@xxxxxxxxxx>
>
>
>
I'll post another version. It's quite a big change, could be
that I miss some points. I didn't split the allocated list into a
separate patch, I think was not clear the purpose, I put more comment
on the commit.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel