[Spice-devel] [PATCH v3 1/8] replay: Record allocations in a GList to handle errors

[Frediano Ziglio]

> 
> Hi,
> 
> On Fri, Sep 16, 2016 at 12:32:54PM +0100, Frediano Ziglio wrote:
> > Allocations are kept into a GList to be able to free in case some
> > errors happened.
> >
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/red-replay-qxl.c | 68
> >  ++++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 56 insertions(+), 12 deletions(-)
> >
> > diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> > index 7ce8f47..7eddaf4 100644
> > --- a/server/red-replay-qxl.c
> > +++ b/server/red-replay-qxl.c
> > @@ -50,6 +50,8 @@ struct SpiceReplay {
> >      GArray *id_free; // free list
> >      int nsurfaces;
> >
> > +    GList *allocated;
> > +
> >      pthread_mutex_t mutex;
> >      pthread_cond_t cond;
> >  };
> > @@ -88,6 +90,20 @@ static replay_t replay_fscanf(SpiceReplay *replay, const
> > char *fmt, ...)
> >      return replay->error ? REPLAY_ERROR : REPLAY_OK;
> >  }
> >
> > +static inline void *replay_malloc(SpiceReplay *replay, size_t size)
> > +{
> > +    void *mem = spice_malloc(size);
> > +    replay->allocated = g_list_prepend(replay->allocated, mem);
> > +    return mem;
> > +}
> > +
> > +static inline void *replay_malloc0(SpiceReplay *replay, size_t size)
> > +{
> > +    void *mem = replay_malloc(replay, size);
> > +    memset(mem, 0, size);
> > +    return mem;
> > +}
> > +
> >  static uint32_t replay_id_get(SpiceReplay *replay, uint32_t id)
> >  {
> >      uint32_t newid = 0;
> > @@ -199,7 +215,7 @@ static replay_t read_binary(SpiceReplay *replay, const
> > char *prefix, size_t *siz
> >          return REPLAY_ERROR;
> >
> >      if (*buf == NULL) {
> > -        *buf = spice_malloc(*size + base_size);
> > +        *buf = replay_malloc(replay, *size + base_size);
> >      }
> >  #if 0
> >      {
> > @@ -211,9 +227,11 @@ static replay_t read_binary(SpiceReplay *replay, const
> > char *prefix, size_t *siz
> >      spice_return_val_if_fail(with_zlib != -1, REPLAY_ERROR);
> >      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;
> >          replay_fread(replay, zlib_buffer, zlib_size);
> >          strm.zalloc = Z_NULL;
> >          strm.zfree = Z_NULL;
> > @@ -241,6 +259,7 @@ 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
> 
> I think we could use a replay_free(replay, zlib_buffer) here.
> I think we might want to use g_list_find() as g_list_remove_link() does
> not warn about element not being on the list and we might want that
> extra check (would be a double free) - this double free might not make
> sense in the above logic but for the rest of the code.
> 
> >      } else {
> >          replay_fread(replay, *buf + base_size, *size);
> > @@ -377,7 +396,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 +417,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]));
> 
> I think you _need_ the handle the qxl realloc in the switch
> SPICE_IMAGE_TYPE_QUIC statement as the pointer might be different.
> 
> 

This is handled in 5/8 if I understood this comment

> >              qp->num_ents = num_ents;
> >              qxl->bitmap.palette = QXLPHYSICAL_FROM_PTR(qp);
> >              replay_fscanf(replay, "unique %"PRIu64"\n", &qp->unique);

Frediano