[Spice-devel] [PATCH] replay: Handle errors in record file

Christophe Fergeau cfergeau at redhat.com
Thu Sep 15 15:21:42 UTC 2016


Hey,

On Mon, Sep 12, 2016 at 12:56:05PM +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.

It would be nice to have this bit in a separate commit

> To check fscanf read all needed information a dummy "%n" is appended
> to any string and the value stored there is tested. This as scanf family
> could return a valid value but not entirely process the string so
> adding a "%n" and checking this was processed make sure all expected
> string is found.

And this one in a separate one too

> The "eof" field is used to mark any error happened, so in most cases
> there is no explicit check beside when this could cause a problem
> (for instance results of replay_fscanf used which would result in
> uninitialised variable usage).

Should we rename that field then? If it's no longer 'end-of-file', but
'error-occurred', 'eof' is very misleading.

> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-replay-qxl.c | 256 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 185 insertions(+), 71 deletions(-)
> 
> Don't remember which version of the patch is.
> If I remember it should fix latests comments.
> 
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index b17c38b..f916fa8 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -49,29 +49,33 @@ 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;
>  };
>  
> -static int replay_fread(SpiceReplay *replay, uint8_t *buf, size_t size)
> +static ssize_t replay_fread(SpiceReplay *replay, uint8_t *buf, size_t size)
>  {
> -    if (replay->eof) {
> -        return 0;
> -    }
> -    if (feof(replay->fd)) {
> +    if (replay->eof
> +        || feof(replay->fd)
> +        || fread(buf, 1, size, replay->fd) != size) {

Really not a big fan of chaining calls in an if, what about something
like this?

if (replay->eof)
  goto error;

if (feof(replay->fd)
  goto error;

if (fread(...) ..)
  goto error;

return size;

error:
  ...



>          replay->eof = 1;
>          return 0;
>      }
> -    return fread(buf, size, 1, replay->fd);
> +    return size;
>  }
>  
>  __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 +86,27 @@ 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) {
>          replay->eof = 1;
>      }
>      return replay->eof ? REPLAY_EOF : REPLAY_OK;
>  }
> +#define replay_fscanf(r, fmt, ...) \
> +    replay_fscanf_check(r, fmt "%n", ## __VA_ARGS__, &r->end_pos)
> +
> +static inline void *replay_malloc(SpiceReplay *replay, size_t size)
> +{
> +    void *p = spice_malloc(size);

Can we get a more expressive name than 'p' ? :) mem, allocation or
allocated_mem or whatever

> +    replay->allocated = g_list_prepend(replay->allocated, p);
> +    return p;
> +}
> +
> +static inline void *replay_malloc0(SpiceReplay *replay, size_t size)
> +{
> +    void *p = spice_malloc0(size);
> +    replay->allocated = g_list_prepend(replay->allocated, p);
> +    return p;
> +}

replay_malloc0 could be:
void *mem = replay_malloc(replay, size);
memset(mem, '\0', size);



>  
>  static uint32_t replay_id_get(SpiceReplay *replay, uint32_t id)
>  {
> @@ -188,18 +208,20 @@ 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];
> -    int with_zlib = -1;
> -    int zlib_size;
> -    uint8_t *zlib_buffer;
> +    char file_prefix[1024];
> +    int with_zlib;
>      z_stream strm;
>  
> -    snprintf(template, sizeof(template), "binary %%d %s %%ld:", prefix);
> -    if (replay_fscanf(replay, template, &with_zlib, size) == REPLAY_EOF)
> +    if (replay_fscanf(replay, "binary %d %1000s %ld:", &with_zlib, file_prefix, size) == REPLAY_EOF) {

file_prefix is 1024 bytes, but you only use 1000 here?

>          return REPLAY_EOF;
> +    }
> +    if (strcmp(file_prefix, prefix) != 0) {
> +        replay->eof = 1;
> +        return REPLAY_EOF;
> +    }
>  
>      if (*buf == NULL) {
> -        *buf = spice_malloc(*size + base_size);
> +        *buf = replay_malloc(replay, *size + base_size);
>      }
>  #if 0
>      {
> @@ -211,10 +233,18 @@ 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;
> +        uint8_t *zlib_buffer;
> +        int zlib_size;
>  
> -        replay_fscanf(replay, "%d:", &zlib_size);
> -        zlib_buffer = spice_malloc(zlib_size);
> -        replay_fread(replay, zlib_buffer, zlib_size);
> +        if (replay_fscanf(replay, "%d:", &zlib_size) == REPLAY_EOF) {
> +            return REPLAY_EOF;
> +        }
> +        zlib_buffer = replay_malloc(replay, zlib_size);
> +        elem = replay->allocated;
> +        if (replay_fread(replay, zlib_buffer, zlib_size) != zlib_size) {

Test is slightly redundant as replay_fread now is going to return either
0 or zlib_size.

> +            return REPLAY_EOF;
> +        }
>          strm.zalloc = Z_NULL;
>          strm.zfree = Z_NULL;
>          strm.opaque = Z_NULL;
> @@ -241,16 +271,16 @@ 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
>      } else {
>          replay_fread(replay, *buf + base_size, *size);
>      }
>  #endif
> -    replay_fscanf(replay, "\n");
> -    return REPLAY_OK;
> +    return replay_fscanf(replay, "\n");
>  }
>  
> -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;
> @@ -258,7 +288,9 @@ static size_t red_replay_data_chunks(SpiceReplay *replay, const char *prefix,
>      size_t next_data_size;
>      QXLDataChunk *cur, *next;
>  
> -    replay_fscanf(replay, "data_chunks %d %zu\n", &count_chunks, &data_size);
> +    if (replay_fscanf(replay, "data_chunks %d %zu\n", &count_chunks, &data_size) == REPLAY_EOF) {
> +        return REPLAY_EOF;
> +    }
>      if (base_size == 0) {
>          base_size = sizeof(QXLDataChunk);
>      }
> @@ -316,19 +348,26 @@ static void red_replay_point16_ptr(SpiceReplay *replay, QXLPoint16 *qxl)
>  
>  static void 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);
> +    if (replay_fscanf(replay, "rect %1000s %d %d %d %d\n", file_prefix, &qxl->top, &qxl->left,
> +                      &qxl->bottom, &qxl->right) == REPLAY_EOF) {
> +        return;
> +    }
> +    if (strcmp(file_prefix, prefix) != 0) {
> +        replay->eof = 1;
> +    }

Fwiw, I'd say the snprintf version was a bit more obvious than this one.

>  }
>  
>  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;

I think this should make sure that NULL is returned if
red_replay_data_chunks() failed (or red_replay_data_chunks() should do
that).
>  }
>  
> @@ -344,9 +383,11 @@ static QXLClipRects *red_replay_clip_rects(SpiceReplay *replay)
>      QXLClipRects *qxl = NULL;
>      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 (replay_fscanf(replay, "num_rects %d\n", &num_rects) == REPLAY_EOF) {
> +        return NULL;
> +    }
> +    if (red_replay_data_chunks(replay, "clip_rects", (uint8_t**)&qxl, sizeof(QXLClipRects)) >= 0)
> +        qxl->num_rects = num_rects;

Same comment here.


>      return qxl;
>  }
>  
> @@ -365,24 +406,29 @@ 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;
>      int has_image;
>  
> -    replay_fscanf(replay, "image %d\n", &has_image);
> +    if (replay_fscanf(replay, "image %d\n", &has_image) == REPLAY_EOF) {
> +        return NULL;
> +    }
>      if (!has_image) {
>          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;
>      replay_fscanf(replay, "descriptor.width %d\n", &qxl->descriptor.width);
> -    replay_fscanf(replay, "descriptor.height %d\n", &qxl->descriptor.height);
> +    if (replay_fscanf(replay, "descriptor.height %d\n", &qxl->descriptor.height) == REPLAY_EOF) {
> +        return NULL;
> +    }
>  
>      switch (qxl->descriptor.type) {
>      case SPICE_IMAGE_TYPE_BITMAP:
> @@ -397,8 +443,10 @@ static QXLImage *red_replay_image(SpiceReplay *replay, uint32_t flags)
>              QXLPalette *qp;
>              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]));
> +            if (replay_fscanf(replay, "qp.num_ents %d\n", &num_ents) == REPLAY_EOF) {
> +                return NULL;
> +            }
> +            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 +461,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;
> @@ -421,13 +469,18 @@ static QXLImage *red_replay_image(SpiceReplay *replay, uint32_t flags)
>          }
>          break;
>      case SPICE_IMAGE_TYPE_SURFACE:
> -        replay_fscanf(replay, "surface_image.surface_id %d\n", &qxl->surface_image.surface_id);
> +        if (replay_fscanf(replay, "surface_image.surface_id %d\n", &qxl->surface_image.surface_id)
> +            == REPLAY_EOF) {
> +            return NULL;
> +        }
>          qxl->surface_image.surface_id = replay_id_get(replay, qxl->surface_image.surface_id);
>          break;
>      case SPICE_IMAGE_TYPE_QUIC:
>          // TODO - make this much nicer (precompute size and allocs, store them during
>          // record, then reread into them. and use MPEG-4).
> -        replay_fscanf(replay, "quic.data_size %d\n", &qxl->quic.data_size);
> +        if (replay_fscanf(replay, "quic.data_size %d\n", &qxl->quic.data_size) == REPLAY_EOF) {
> +            return NULL;
> +        }
>          qxl = realloc(qxl, sizeof(QXLImageDescriptor) + sizeof(QXLQUICData) +
>                        qxl->quic.data_size);
>          size = red_replay_data_chunks(replay, "quic.data", (uint8_t**)&qxl->quic.data, 0);
> @@ -468,7 +521,10 @@ static void red_replay_image_free(SpiceReplay *replay, QXLPHYSICAL p, uint32_t f
>  
>  static void red_replay_brush_ptr(SpiceReplay *replay, QXLBrush *qxl, uint32_t flags)
>  {
> -    replay_fscanf(replay, "type %d\n", &qxl->type);
> +    if (replay_fscanf(replay, "type %d\n", &qxl->type) == REPLAY_EOF) {
> +        return;
> +    }
> +
>      switch (qxl->type) {
>      case SPICE_BRUSH_TYPE_SOLID:
>          replay_fscanf(replay, "u.color %d\n", &qxl->u.color);
> @@ -632,7 +688,10 @@ static void red_replay_stroke_ptr(SpiceReplay *replay, QXLStroke *qxl, uint32_t
>      int temp;
>  
>      qxl->path = QXLPHYSICAL_FROM_PTR(red_replay_path(replay));
> -    replay_fscanf(replay, "attr.flags %d\n", &temp); qxl->attr.flags = temp;
> +    if (replay_fscanf(replay, "attr.flags %d\n", &temp) == REPLAY_EOF) {
> +        return;
> +    }
> +    qxl->attr.flags = temp;
>      if (qxl->attr.flags & SPICE_LINE_FLAGS_STYLED) {
>          size_t size;
>  
> @@ -659,17 +718,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);
> +    }

else { return NULL; }

>      return qxl;
>  }
>  
> @@ -729,7 +790,9 @@ static void red_replay_invers_free(SpiceReplay *replay, QXLInvers *qxl, uint32_t
>  
>  static void red_replay_clip_ptr(SpiceReplay *replay, QXLClip *qxl)
>  {
> -    replay_fscanf(replay, "type %d\n", &qxl->type);
> +    if (replay_fscanf(replay, "type %d\n", &qxl->type) == REPLAY_EOF) {
> +        return;
> +    }
>      switch (qxl->type) {
>      case SPICE_CLIP_TYPE_RECTS:
>          qxl->data = QXLPHYSICAL_FROM_PTR(red_replay_clip_rects(replay));
> @@ -759,7 +822,7 @@ static uint8_t *red_replay_transform(SpiceReplay *replay)
>  
>  static void red_replay_composite_ptr(SpiceReplay *replay, QXLComposite *qxl, uint32_t flags)
>  {
> -    int enabled;
> +    int enabled = 0;
>  
>      replay_fscanf(replay, "flags %d\n", &qxl->flags);
>      qxl->src = QXLPHYSICAL_FROM_PTR(red_replay_image(replay, flags));
> @@ -788,7 +851,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;
>  
> @@ -798,16 +861,22 @@ static QXLDrawable *red_replay_native_drawable(SpiceReplay *replay, uint32_t fla
>      replay_fscanf(replay, "mm_time %d\n", &qxl->mm_time);
>      replay_fscanf(replay, "self_bitmap %d\n", &temp); qxl->self_bitmap = temp;
>      red_replay_rect_ptr(replay, "self_bitmap_area", &qxl->self_bitmap_area);
> -    replay_fscanf(replay, "surface_id %d\n", &qxl->surface_id);
> +    if (replay_fscanf(replay, "surface_id %d\n", &qxl->surface_id) == REPLAY_EOF) {
> +        return NULL;
> +    }
>      qxl->surface_id = replay_id_get(replay, qxl->surface_id);
>  
>      for (i = 0; i < 3; i++) {
> -        replay_fscanf(replay, "surfaces_dest %d\n", &qxl->surfaces_dest[i]);
> +        if (replay_fscanf(replay, "surfaces_dest %d\n", &qxl->surfaces_dest[i]) == REPLAY_EOF) {
> +            return NULL;
> +        }
>          qxl->surfaces_dest[i] = replay_id_get(replay, qxl->surfaces_dest[i]);
>          red_replay_rect_ptr(replay, "surfaces_rects", &qxl->surfaces_rects[i]);
>      }
>  
> -    replay_fscanf(replay, "type %d\n", &temp); qxl->type = temp;
> +    if (replay_fscanf(replay, "type %d\n", &temp) == REPLAY_EOF)
> +        return NULL;
> +    qxl->type = temp;
>      switch (qxl->type) {
>      case QXL_DRAW_ALPHA_BLEND:
>          red_replay_alpha_blend_ptr(replay, &qxl->u.alpha_blend, flags);
> @@ -857,7 +926,11 @@ 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 +992,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);
> @@ -929,7 +1002,10 @@ static QXLCompatDrawable *red_replay_compat_drawable(SpiceReplay *replay, uint32
>      replay_fscanf(replay, "bitmap_offset %d\n", &temp); qxl->bitmap_offset = temp;
>      red_replay_rect_ptr(replay, "bitmap_area", &qxl->bitmap_area);
>  
> -    replay_fscanf(replay, "type %d\n", &temp); qxl->type = temp;
> +    if (replay_fscanf(replay, "type %d\n", &temp) == REPLAY_EOF) {
> +        return NULL;
> +    }
> +    qxl->type = temp;
>      switch (qxl->type) {
>      case QXL_DRAW_ALPHA_BLEND:
>          red_replay_alpha_blend_ptr_compat(replay, &qxl->u.alpha_blend, flags);
> @@ -976,14 +1052,15 @@ 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,12 +1071,14 @@ 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);
>      replay_fscanf(replay, "update_id %d\n", &qxl->update_id);
> -    replay_fscanf(replay, "surface_id %d\n", &qxl->surface_id);
> +    if (replay_fscanf(replay, "surface_id %d\n", &qxl->surface_id) == REPLAY_EOF) {
> +        return NULL;
> +    }
>      qxl->surface_id = replay_id_get(replay, qxl->surface_id);
>  
>      return qxl;
> @@ -1024,14 +1103,19 @@ static QXLSurfaceCmd *red_replay_surface_cmd(SpiceReplay *replay)
>      replay_fscanf(replay, "surface_cmd\n");
>      replay_fscanf(replay, "surface_id %d\n", &qxl->surface_id);
>      replay_fscanf(replay, "type %d\n", &temp); qxl->type = temp;
> -    replay_fscanf(replay, "flags %d\n", &qxl->flags);
> +    if (replay_fscanf(replay, "flags %d\n", &qxl->flags) == REPLAY_EOF) {
> +        return NULL;
> +    }

Hmm, so if I followed correctly, only the last _fscanf is tested for
errors because replay->eof is going to be set if any failed, and
replay_fscanf() will always return an error when this is set?
If yes, I think I'd never check replay_fscanf() return value, and just
add an explicit check for replay->eof (or a helper doing this check)
after the last fscanf.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160915/1bd0c949/attachment-0001.sig>


More information about the Spice-devel mailing list