[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-stable] [Qemu-devel] [PATCH] rbd: avoid qemu_rbd_snap_list() m
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-stable] [Qemu-devel] [PATCH] rbd: avoid qemu_rbd_snap_list() memory leak when no snapshots |
Date: |
Wed, 25 Sep 2013 15:58:02 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, Sep 25, 2013 at 10:06:11AM +0200, Kevin Wolf wrote:
> Am 25.09.2013 um 02:28 hat Michael Roth geschrieben:
> > Quoting Stefan Hajnoczi (2013-08-14 07:13:52)
> > > When there are no snapshots qemu_rbd_snap_list() returns 0 and the
> > > snapshot table pointer is NULL. Don't forget to free the snaps buffer
> > > we allocated for librbd rbd_snap_list().
> > >
> > > Cc: address@hidden
> > > Signed-off-by: Stefan Hajnoczi <address@hidden>
> >
> > Ping for 1.6.1
>
> Applied it to the block branch for now, but...
>
> > > ---
> > > block/rbd.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/block/rbd.c b/block/rbd.c
> > > index cb71751..4e26fea 100644
> > > --- a/block/rbd.c
> > > +++ b/block/rbd.c
> > > @@ -934,7 +934,7 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
> > > do {
> > > snaps = g_malloc(sizeof(*snaps) * max_snaps);
> > > snap_count = rbd_snap_list(s->image, snaps, &max_snaps);
> > > - if (snap_count < 0) {
> > > + if (snap_count <= 0) {
> > > g_free(snaps);
> > > }
> > > } while (snap_count == -ERANGE);
>
> ...I think this isn't a complete fix. In the successful case we still
> leak snaps. The g_free() should probably be moved to after the done:
> label in a v2 of the patch.
You are right. I'm sending a v2. rbd_snap_list_end() does not free
snaps itself, only the strings that snaps[i].name points to. Therefore
we need to free snaps.