* [Qemu-devel] [PATCH] rbd: avoid qemu_rbd_snap_list() memory leak when no snapshots
@ 2013-08-14 12:13 Stefan Hajnoczi
2013-09-25 0:28 ` Michael Roth
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Hajnoczi @ 2013-08-14 12:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Josh Durgin, qemu-stable, Stefan Hajnoczi
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: qemu-stable@nongnu.org
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] rbd: avoid qemu_rbd_snap_list() memory leak when no snapshots
2013-08-14 12:13 [Qemu-devel] [PATCH] rbd: avoid qemu_rbd_snap_list() memory leak when no snapshots Stefan Hajnoczi
@ 2013-09-25 0:28 ` Michael Roth
2013-09-25 8:06 ` Kevin Wolf
0 siblings, 1 reply; 4+ messages in thread
From: Michael Roth @ 2013-09-25 0:28 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel; +Cc: Kevin Wolf, Josh Durgin, qemu-stable
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: qemu-stable@nongnu.org
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Ping for 1.6.1
> ---
> 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);
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] rbd: avoid qemu_rbd_snap_list() memory leak when no snapshots
2013-09-25 0:28 ` Michael Roth
@ 2013-09-25 8:06 ` Kevin Wolf
2013-09-25 13:58 ` Stefan Hajnoczi
0 siblings, 1 reply; 4+ messages in thread
From: Kevin Wolf @ 2013-09-25 8:06 UTC (permalink / raw)
To: Michael Roth; +Cc: Josh Durgin, qemu-devel, Stefan Hajnoczi, qemu-stable
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: qemu-stable@nongnu.org
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> 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.
Kevin
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] rbd: avoid qemu_rbd_snap_list() memory leak when no snapshots
2013-09-25 8:06 ` Kevin Wolf
@ 2013-09-25 13:58 ` Stefan Hajnoczi
0 siblings, 0 replies; 4+ messages in thread
From: Stefan Hajnoczi @ 2013-09-25 13:58 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Josh Durgin, qemu-stable, Michael Roth, qemu-devel
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: qemu-stable@nongnu.org
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >
> > 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-25 13:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-14 12:13 [Qemu-devel] [PATCH] rbd: avoid qemu_rbd_snap_list() memory leak when no snapshots Stefan Hajnoczi
2013-09-25 0:28 ` Michael Roth
2013-09-25 8:06 ` Kevin Wolf
2013-09-25 13:58 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).