| Message ID | 1321978604-4858-1-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 22.11.2011 17:16, schrieb Kevin Wolf: > The block map is allocated in vdi_open, but was never freed. > > Signed-off-by: Kevin Wolf<kwolf@redhat.com> > --- > Applies on top if the migration blocker series. > > block/vdi.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/block/vdi.c b/block/vdi.c > index 7dda522..02da6b4 100644 > --- a/block/vdi.c > +++ b/block/vdi.c > @@ -949,6 +949,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options) > static void vdi_close(BlockDriverState *bs) > { > BDRVVdiState *s = bs->opaque; > + > + g_free(s->bmap); > + > migrate_del_blocker(s->migration_blocker); > error_free(s->migration_blocker); > } If vdi_close is called after a jump to label fail_free_bmap, g_free(s->bmap) will be called twice. Setting s->bmap = NULL after g_free in fail_free_bmap would be safer. Otherwise your patch is fine. If you send an update, you can add Reviewed-by: Stefan Weil <sw@weilnetz.de> Regards, Stefan Weil
Am 22.11.2011 19:37, schrieb Stefan Weil: > Am 22.11.2011 17:16, schrieb Kevin Wolf: >> The block map is allocated in vdi_open, but was never freed. >> >> Signed-off-by: Kevin Wolf<kwolf@redhat.com> >> --- >> Applies on top if the migration blocker series. >> >> block/vdi.c | 3 +++ >> 1 files changed, 3 insertions(+), 0 deletions(-) >> >> diff --git a/block/vdi.c b/block/vdi.c >> index 7dda522..02da6b4 100644 >> --- a/block/vdi.c >> +++ b/block/vdi.c >> @@ -949,6 +949,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options) >> static void vdi_close(BlockDriverState *bs) >> { >> BDRVVdiState *s = bs->opaque; >> + >> + g_free(s->bmap); >> + >> migrate_del_blocker(s->migration_blocker); >> error_free(s->migration_blocker); >> } > > If vdi_close is called after a jump to label fail_free_bmap, > g_free(s->bmap) will be called twice. bdrv_close() may only ever be called on a successfully opened image. In fact, block.c already ensures this. The BDRVVdiState would already be freed in the failing bdrv_open_common() and bs->drv would be set to NULL, so that an invalid bdrv_close() would end up doing nothing. > Setting s->bmap = NULL after g_free in fail_free_bmap > would be safer. > > Otherwise your patch is fine. If you send an update, you > can add > > Reviewed-by: Stefan Weil <sw@weilnetz.de> Thanks. Kevin
diff --git a/block/vdi.c b/block/vdi.c index 7dda522..02da6b4 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -949,6 +949,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options) static void vdi_close(BlockDriverState *bs) { BDRVVdiState *s = bs->opaque; + + g_free(s->bmap); + migrate_del_blocker(s->migration_blocker); error_free(s->migration_blocker); }
The block map is allocated in vdi_open, but was never freed. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- Applies on top if the migration blocker series. block/vdi.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)