| Message ID | 1378473154-30057-3-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 09/06 15:12, Max Reitz wrote: > Add the new ImageInfoSpecific type also to BlockDriverInfo. > > To prevent memory leaks, this field has to be initialized to NULL every > time before calling bdrv_get_info and qapi_free_ImageInfoSpecific has to > be called on it when the BlockDriverInfo object is no longer required. > I don't understand here. I think bdi is always passed into bdrv_get_info() uninitialized and in bdrv_get_info() there is: memset(bdi, 0, sizeof(*bdi)); before passing it on to driver, so it's always set to NULL. And why pointer, not a member to save a free() call? > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 3 ++- > block/mirror.c | 6 ++++-- > block/qapi.c | 6 +++++- > include/block/block.h | 2 ++ > qemu-img.c | 3 ++- > qemu-io-cmds.c | 6 +++++- > 6 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/block.c b/block.c > index 26639e8..1a5d2a4 100644 > --- a/block.c > +++ b/block.c > @@ -1921,7 +1921,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, > int64_t *cluster_sector_num, > int *cluster_nb_sectors) > { > - BlockDriverInfo bdi; > + BlockDriverInfo bdi = { .format_specific = NULL }; > > if (bdrv_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) { > *cluster_sector_num = sector_num; > @@ -1932,6 +1932,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, > *cluster_nb_sectors = QEMU_ALIGN_UP(sector_num - *cluster_sector_num + > nb_sectors, c); > } > + qapi_free_ImageInfoSpecific(bdi.format_specific); > } > > static bool tracked_request_overlaps(BdrvTrackedRequest *req, > diff --git a/block/mirror.c b/block/mirror.c > index 86de458..cfef7e9 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -295,7 +295,7 @@ static void coroutine_fn mirror_run(void *opaque) > BlockDriverState *bs = s->common.bs; > int64_t sector_num, end, sectors_per_chunk, length; > uint64_t last_pause_ns; > - BlockDriverInfo bdi; > + BlockDriverInfo bdi = { .format_specific = NULL }; > char backing_filename[1024]; > int ret = 0; > int n; > @@ -325,6 +325,7 @@ static void coroutine_fn mirror_run(void *opaque) > s->buf_size = MAX(s->buf_size, bdi.cluster_size); > s->cow_bitmap = bitmap_new(length); > } > + qapi_free_ImageInfoSpecific(bdi.format_specific); > } > > end = s->common.len >> BDRV_SECTOR_BITS; > @@ -544,13 +545,14 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, > if (granularity == 0) { > /* Choose the default granularity based on the target file's cluster > * size, clamped between 4k and 64k. */ > - BlockDriverInfo bdi; > + BlockDriverInfo bdi = { .format_specific = NULL }; > if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) { > granularity = MAX(4096, bdi.cluster_size); > granularity = MIN(65536, granularity); > } else { > granularity = 65536; > } > + qapi_free_ImageInfoSpecific(bdi.format_specific); > } > > assert ((granularity & (granularity - 1)) == 0); > diff --git a/block/qapi.c b/block/qapi.c > index a4bc411..f13fbd5 100644 > --- a/block/qapi.c > +++ b/block/qapi.c > @@ -110,7 +110,7 @@ void bdrv_query_image_info(BlockDriverState *bs, > uint64_t total_sectors; > const char *backing_filename; > char backing_filename2[1024]; > - BlockDriverInfo bdi; > + BlockDriverInfo bdi = { .format_specific = NULL }; > int ret; > Error *err = NULL; > ImageInfo *info = g_new0(ImageInfo, 1); > @@ -133,6 +133,10 @@ void bdrv_query_image_info(BlockDriverState *bs, > } > info->dirty_flag = bdi.is_dirty; > info->has_dirty_flag = true; > + if (bdi.format_specific) { > + info->format_specific = bdi.format_specific; > + info->has_format_specific = true; > + } > } > backing_filename = bs->backing_file; > if (backing_filename[0] != '\0') { > diff --git a/include/block/block.h b/include/block/block.h > index e6b391c..85e9703 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -18,6 +18,8 @@ typedef struct BlockDriverInfo { > /* offset at which the VM state can be saved (0 if not possible) */ > int64_t vm_state_offset; > bool is_dirty; > + /* additional information; NULL if none */ > + ImageInfoSpecific *format_specific; > } BlockDriverInfo; > > typedef struct BlockFragInfo { > diff --git a/qemu-img.c b/qemu-img.c > index b9a848d..ec1ecca 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1125,7 +1125,7 @@ static int img_convert(int argc, char **argv) > uint64_t bs_sectors; > uint8_t * buf = NULL; > const uint8_t *buf1; > - BlockDriverInfo bdi; > + BlockDriverInfo bdi = { .format_specific = NULL }; > QEMUOptionParameter *param = NULL, *create_options = NULL; > QEMUOptionParameter *out_baseimg_param; > char *options = NULL; > @@ -1369,6 +1369,7 @@ static int img_convert(int argc, char **argv) > error_report("could not get block driver info"); > goto out; > } > + qapi_free_ImageInfoSpecific(bdi.format_specific); > cluster_size = bdi.cluster_size; > if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) { > error_report("invalid cluster size"); > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > index f91b6c4..563dd40 100644 > --- a/qemu-io-cmds.c > +++ b/qemu-io-cmds.c > @@ -1677,7 +1677,7 @@ static const cmdinfo_t length_cmd = { > > static int info_f(BlockDriverState *bs, int argc, char **argv) > { > - BlockDriverInfo bdi; > + BlockDriverInfo bdi = { .format_specific = NULL }; > char s1[64], s2[64]; > int ret; > > @@ -1699,6 +1699,10 @@ static int info_f(BlockDriverState *bs, int argc, char **argv) > printf("cluster size: %s\n", s1); > printf("vm state offset: %s\n", s2); > > + if (bdi.format_specific) { > + qapi_free_ImageInfoSpecific(bdi.format_specific); > + } > + > return 0; > } > > -- > 1.8.3.1 > >
On 2013-09-10 05:26, Fam Zheng wrote: > On Fri, 09/06 15:12, Max Reitz wrote: >> Add the new ImageInfoSpecific type also to BlockDriverInfo. >> >> To prevent memory leaks, this field has to be initialized to NULL every >> time before calling bdrv_get_info and qapi_free_ImageInfoSpecific has to >> be called on it when the BlockDriverInfo object is no longer required. >> > I don't understand here. I think bdi is always passed into bdrv_get_info() > uninitialized and in bdrv_get_info() there is: > > memset(bdi, 0, sizeof(*bdi)); > > before passing it on to driver, so it's always set to NULL. Oh, you're right, I missed that. Thanks! > And why pointer, not a member to save a free() call? As far as I understand it (and if I don't miss anything again), ImageInfoSpecific is a auto-generated QAPI type, so it may contain pointers to other types anyway (as it does in the case of QCow2, which is the only driver where I have implemented this new field at all; in that case, the compatiblity level is a string), therefore we always need some function to clean up the data referenced by ImageInfoSpecific; qapi_free_ImageInfoSpecific is the perfect one for this, but it takes a heap pointer. Hence, I think using a pointer (to a heap-allocated object) is easier in this case, since the QAPI clean-up function assumes this case. >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 3 ++- >> block/mirror.c | 6 ++++-- >> block/qapi.c | 6 +++++- >> include/block/block.h | 2 ++ >> qemu-img.c | 3 ++- >> qemu-io-cmds.c | 6 +++++- >> 6 files changed, 20 insertions(+), 6 deletions(-) >> >> diff --git a/block.c b/block.c >> index 26639e8..1a5d2a4 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1921,7 +1921,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, >> int64_t *cluster_sector_num, >> int *cluster_nb_sectors) >> { >> - BlockDriverInfo bdi; >> + BlockDriverInfo bdi = { .format_specific = NULL }; >> >> if (bdrv_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) { >> *cluster_sector_num = sector_num; >> @@ -1932,6 +1932,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, >> *cluster_nb_sectors = QEMU_ALIGN_UP(sector_num - *cluster_sector_num + >> nb_sectors, c); >> } >> + qapi_free_ImageInfoSpecific(bdi.format_specific); >> } >> >> static bool tracked_request_overlaps(BdrvTrackedRequest *req, >> diff --git a/block/mirror.c b/block/mirror.c >> index 86de458..cfef7e9 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -295,7 +295,7 @@ static void coroutine_fn mirror_run(void *opaque) >> BlockDriverState *bs = s->common.bs; >> int64_t sector_num, end, sectors_per_chunk, length; >> uint64_t last_pause_ns; >> - BlockDriverInfo bdi; >> + BlockDriverInfo bdi = { .format_specific = NULL }; >> char backing_filename[1024]; >> int ret = 0; >> int n; >> @@ -325,6 +325,7 @@ static void coroutine_fn mirror_run(void *opaque) >> s->buf_size = MAX(s->buf_size, bdi.cluster_size); >> s->cow_bitmap = bitmap_new(length); >> } >> + qapi_free_ImageInfoSpecific(bdi.format_specific); >> } >> >> end = s->common.len >> BDRV_SECTOR_BITS; >> @@ -544,13 +545,14 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, >> if (granularity == 0) { >> /* Choose the default granularity based on the target file's cluster >> * size, clamped between 4k and 64k. */ >> - BlockDriverInfo bdi; >> + BlockDriverInfo bdi = { .format_specific = NULL }; >> if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) { >> granularity = MAX(4096, bdi.cluster_size); >> granularity = MIN(65536, granularity); >> } else { >> granularity = 65536; >> } >> + qapi_free_ImageInfoSpecific(bdi.format_specific); >> } >> >> assert ((granularity & (granularity - 1)) == 0); >> diff --git a/block/qapi.c b/block/qapi.c >> index a4bc411..f13fbd5 100644 >> --- a/block/qapi.c >> +++ b/block/qapi.c >> @@ -110,7 +110,7 @@ void bdrv_query_image_info(BlockDriverState *bs, >> uint64_t total_sectors; >> const char *backing_filename; >> char backing_filename2[1024]; >> - BlockDriverInfo bdi; >> + BlockDriverInfo bdi = { .format_specific = NULL }; >> int ret; >> Error *err = NULL; >> ImageInfo *info = g_new0(ImageInfo, 1); >> @@ -133,6 +133,10 @@ void bdrv_query_image_info(BlockDriverState *bs, >> } >> info->dirty_flag = bdi.is_dirty; >> info->has_dirty_flag = true; >> + if (bdi.format_specific) { >> + info->format_specific = bdi.format_specific; >> + info->has_format_specific = true; >> + } >> } >> backing_filename = bs->backing_file; >> if (backing_filename[0] != '\0') { >> diff --git a/include/block/block.h b/include/block/block.h >> index e6b391c..85e9703 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -18,6 +18,8 @@ typedef struct BlockDriverInfo { >> /* offset at which the VM state can be saved (0 if not possible) */ >> int64_t vm_state_offset; >> bool is_dirty; >> + /* additional information; NULL if none */ >> + ImageInfoSpecific *format_specific; >> } BlockDriverInfo; >> >> typedef struct BlockFragInfo { >> diff --git a/qemu-img.c b/qemu-img.c >> index b9a848d..ec1ecca 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -1125,7 +1125,7 @@ static int img_convert(int argc, char **argv) >> uint64_t bs_sectors; >> uint8_t * buf = NULL; >> const uint8_t *buf1; >> - BlockDriverInfo bdi; >> + BlockDriverInfo bdi = { .format_specific = NULL }; >> QEMUOptionParameter *param = NULL, *create_options = NULL; >> QEMUOptionParameter *out_baseimg_param; >> char *options = NULL; >> @@ -1369,6 +1369,7 @@ static int img_convert(int argc, char **argv) >> error_report("could not get block driver info"); >> goto out; >> } >> + qapi_free_ImageInfoSpecific(bdi.format_specific); >> cluster_size = bdi.cluster_size; >> if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) { >> error_report("invalid cluster size"); >> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c >> index f91b6c4..563dd40 100644 >> --- a/qemu-io-cmds.c >> +++ b/qemu-io-cmds.c >> @@ -1677,7 +1677,7 @@ static const cmdinfo_t length_cmd = { >> >> static int info_f(BlockDriverState *bs, int argc, char **argv) >> { >> - BlockDriverInfo bdi; >> + BlockDriverInfo bdi = { .format_specific = NULL }; >> char s1[64], s2[64]; >> int ret; >> >> @@ -1699,6 +1699,10 @@ static int info_f(BlockDriverState *bs, int argc, char **argv) >> printf("cluster size: %s\n", s1); >> printf("vm state offset: %s\n", s2); >> >> + if (bdi.format_specific) { >> + qapi_free_ImageInfoSpecific(bdi.format_specific); >> + } >> + >> return 0; >> } >> >> -- >> 1.8.3.1 >> >> Max
On Tue, 09/10 09:22, Max Reitz wrote: > On 2013-09-10 05:26, Fam Zheng wrote: > >On Fri, 09/06 15:12, Max Reitz wrote: > >>Add the new ImageInfoSpecific type also to BlockDriverInfo. > >> > >>To prevent memory leaks, this field has to be initialized to NULL every > >>time before calling bdrv_get_info and qapi_free_ImageInfoSpecific has to > >>be called on it when the BlockDriverInfo object is no longer required. > >> > >I don't understand here. I think bdi is always passed into bdrv_get_info() > >uninitialized and in bdrv_get_info() there is: > > > > memset(bdi, 0, sizeof(*bdi)); > > > >before passing it on to driver, so it's always set to NULL. > Oh, you're right, I missed that. Thanks! > > >And why pointer, not a member to save a free() call? > As far as I understand it (and if I don't miss anything again), > ImageInfoSpecific is a auto-generated QAPI type, so it may contain > pointers to other types anyway (as it does in the case of QCow2, > which is the only driver where I have implemented this new field at > all; in that case, the compatiblity level is a string), therefore we > always need some function to clean up the data referenced by > ImageInfoSpecific; qapi_free_ImageInfoSpecific is the perfect one > for this, but it takes a heap pointer. > > Hence, I think using a pointer (to a heap-allocated object) is > easier in this case, since the QAPI clean-up function assumes this > case. > OK, learning this from you. Since this is allocated in bdrv_get_info() (from the caller PoV), and, the info requires releasing after use, a bdrv_put_info() may be a good function to do it, instead of directly operate on the field everywhere. > >>Signed-off-by: Max Reitz <mreitz@redhat.com> > >>--- > >> block.c | 3 ++- > >> block/mirror.c | 6 ++++-- > >> block/qapi.c | 6 +++++- > >> include/block/block.h | 2 ++ > >> qemu-img.c | 3 ++- > >> qemu-io-cmds.c | 6 +++++- > >> 6 files changed, 20 insertions(+), 6 deletions(-) > >> > >>diff --git a/block.c b/block.c > >>index 26639e8..1a5d2a4 100644 > >>--- a/block.c > >>+++ b/block.c > >>@@ -1921,7 +1921,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, > >> int64_t *cluster_sector_num, > >> int *cluster_nb_sectors) > >> { > >>- BlockDriverInfo bdi; > >>+ BlockDriverInfo bdi = { .format_specific = NULL }; > >> if (bdrv_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) { > >> *cluster_sector_num = sector_num; > >>@@ -1932,6 +1932,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, > >> *cluster_nb_sectors = QEMU_ALIGN_UP(sector_num - *cluster_sector_num + > >> nb_sectors, c); > >> } > >>+ qapi_free_ImageInfoSpecific(bdi.format_specific); > >> } > >> static bool tracked_request_overlaps(BdrvTrackedRequest *req, > >>diff --git a/block/mirror.c b/block/mirror.c > >>index 86de458..cfef7e9 100644 > >>--- a/block/mirror.c > >>+++ b/block/mirror.c > >>@@ -295,7 +295,7 @@ static void coroutine_fn mirror_run(void *opaque) > >> BlockDriverState *bs = s->common.bs; > >> int64_t sector_num, end, sectors_per_chunk, length; > >> uint64_t last_pause_ns; > >>- BlockDriverInfo bdi; > >>+ BlockDriverInfo bdi = { .format_specific = NULL }; > >> char backing_filename[1024]; > >> int ret = 0; > >> int n; > >>@@ -325,6 +325,7 @@ static void coroutine_fn mirror_run(void *opaque) > >> s->buf_size = MAX(s->buf_size, bdi.cluster_size); > >> s->cow_bitmap = bitmap_new(length); > >> } > >>+ qapi_free_ImageInfoSpecific(bdi.format_specific); > >> } > >> end = s->common.len >> BDRV_SECTOR_BITS; > >>@@ -544,13 +545,14 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, > >> if (granularity == 0) { > >> /* Choose the default granularity based on the target file's cluster > >> * size, clamped between 4k and 64k. */ > >>- BlockDriverInfo bdi; > >>+ BlockDriverInfo bdi = { .format_specific = NULL }; > >> if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) { > >> granularity = MAX(4096, bdi.cluster_size); > >> granularity = MIN(65536, granularity); > >> } else { > >> granularity = 65536; > >> } > >>+ qapi_free_ImageInfoSpecific(bdi.format_specific); > >> } > >> assert ((granularity & (granularity - 1)) == 0); > >>diff --git a/block/qapi.c b/block/qapi.c > >>index a4bc411..f13fbd5 100644 > >>--- a/block/qapi.c > >>+++ b/block/qapi.c > >>@@ -110,7 +110,7 @@ void bdrv_query_image_info(BlockDriverState *bs, > >> uint64_t total_sectors; > >> const char *backing_filename; > >> char backing_filename2[1024]; > >>- BlockDriverInfo bdi; > >>+ BlockDriverInfo bdi = { .format_specific = NULL }; > >> int ret; > >> Error *err = NULL; > >> ImageInfo *info = g_new0(ImageInfo, 1); > >>@@ -133,6 +133,10 @@ void bdrv_query_image_info(BlockDriverState *bs, > >> } > >> info->dirty_flag = bdi.is_dirty; > >> info->has_dirty_flag = true; > >>+ if (bdi.format_specific) { > >>+ info->format_specific = bdi.format_specific; > >>+ info->has_format_specific = true; > >>+ } > >> } > >> backing_filename = bs->backing_file; > >> if (backing_filename[0] != '\0') { > >>diff --git a/include/block/block.h b/include/block/block.h > >>index e6b391c..85e9703 100644 > >>--- a/include/block/block.h > >>+++ b/include/block/block.h > >>@@ -18,6 +18,8 @@ typedef struct BlockDriverInfo { > >> /* offset at which the VM state can be saved (0 if not possible) */ > >> int64_t vm_state_offset; > >> bool is_dirty; > >>+ /* additional information; NULL if none */ > >>+ ImageInfoSpecific *format_specific; > >> } BlockDriverInfo; > >> typedef struct BlockFragInfo { > >>diff --git a/qemu-img.c b/qemu-img.c > >>index b9a848d..ec1ecca 100644 > >>--- a/qemu-img.c > >>+++ b/qemu-img.c > >>@@ -1125,7 +1125,7 @@ static int img_convert(int argc, char **argv) > >> uint64_t bs_sectors; > >> uint8_t * buf = NULL; > >> const uint8_t *buf1; > >>- BlockDriverInfo bdi; > >>+ BlockDriverInfo bdi = { .format_specific = NULL }; > >> QEMUOptionParameter *param = NULL, *create_options = NULL; > >> QEMUOptionParameter *out_baseimg_param; > >> char *options = NULL; > >>@@ -1369,6 +1369,7 @@ static int img_convert(int argc, char **argv) > >> error_report("could not get block driver info"); > >> goto out; > >> } > >>+ qapi_free_ImageInfoSpecific(bdi.format_specific); > >> cluster_size = bdi.cluster_size; > >> if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) { > >> error_report("invalid cluster size"); > >>diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > >>index f91b6c4..563dd40 100644 > >>--- a/qemu-io-cmds.c > >>+++ b/qemu-io-cmds.c > >>@@ -1677,7 +1677,7 @@ static const cmdinfo_t length_cmd = { > >> static int info_f(BlockDriverState *bs, int argc, char **argv) > >> { > >>- BlockDriverInfo bdi; > >>+ BlockDriverInfo bdi = { .format_specific = NULL }; > >> char s1[64], s2[64]; > >> int ret; > >>@@ -1699,6 +1699,10 @@ static int info_f(BlockDriverState *bs, int argc, char **argv) > >> printf("cluster size: %s\n", s1); > >> printf("vm state offset: %s\n", s2); > >>+ if (bdi.format_specific) { > >>+ qapi_free_ImageInfoSpecific(bdi.format_specific); > >>+ } > >>+ > >> return 0; > >> } > >>-- > >>1.8.3.1 > >> > >> > > Max
On 2013-09-10 09:37, Fam Zheng wrote: > On Tue, 09/10 09:22, Max Reitz wrote: >> On 2013-09-10 05:26, Fam Zheng wrote: >>> On Fri, 09/06 15:12, Max Reitz wrote: >>>> Add the new ImageInfoSpecific type also to BlockDriverInfo. >>>> >>>> To prevent memory leaks, this field has to be initialized to NULL every >>>> time before calling bdrv_get_info and qapi_free_ImageInfoSpecific has to >>>> be called on it when the BlockDriverInfo object is no longer required. >>>> >>> I don't understand here. I think bdi is always passed into bdrv_get_info() >>> uninitialized and in bdrv_get_info() there is: >>> >>> memset(bdi, 0, sizeof(*bdi)); >>> >>> before passing it on to driver, so it's always set to NULL. >> Oh, you're right, I missed that. Thanks! >> >>> And why pointer, not a member to save a free() call? >> As far as I understand it (and if I don't miss anything again), >> ImageInfoSpecific is a auto-generated QAPI type, so it may contain >> pointers to other types anyway (as it does in the case of QCow2, >> which is the only driver where I have implemented this new field at >> all; in that case, the compatiblity level is a string), therefore we >> always need some function to clean up the data referenced by >> ImageInfoSpecific; qapi_free_ImageInfoSpecific is the perfect one >> for this, but it takes a heap pointer. >> >> Hence, I think using a pointer (to a heap-allocated object) is >> easier in this case, since the QAPI clean-up function assumes this >> case. >> > OK, learning this from you. Since this is allocated in bdrv_get_info() (from > the caller PoV), and, the info requires releasing after use, a bdrv_put_info() > may be a good function to do it, instead of directly operate on the field > everywhere. You mean a function for filling the ImageInfoSpecific field? Hm, we can't really use parameters, since every driver would then require its own function (which, imo, would defeat the purpose); the only thing I can imagine right now is a function which converts a JSON description to the object; however, this would again require proper escaping for strings and conversion to strings for non-string types, so I doubt whether this would really help... >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>> --- >>>> block.c | 3 ++- >>>> block/mirror.c | 6 ++++-- >>>> block/qapi.c | 6 +++++- >>>> include/block/block.h | 2 ++ >>>> qemu-img.c | 3 ++- >>>> qemu-io-cmds.c | 6 +++++- >>>> 6 files changed, 20 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/block.c b/block.c >>>> index 26639e8..1a5d2a4 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -1921,7 +1921,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, >>>> int64_t *cluster_sector_num, >>>> int *cluster_nb_sectors) >>>> { >>>> - BlockDriverInfo bdi; >>>> + BlockDriverInfo bdi = { .format_specific = NULL }; >>>> if (bdrv_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) { >>>> *cluster_sector_num = sector_num; >>>> @@ -1932,6 +1932,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, >>>> *cluster_nb_sectors = QEMU_ALIGN_UP(sector_num - *cluster_sector_num + >>>> nb_sectors, c); >>>> } >>>> + qapi_free_ImageInfoSpecific(bdi.format_specific); >>>> } >>>> static bool tracked_request_overlaps(BdrvTrackedRequest *req, >>>> diff --git a/block/mirror.c b/block/mirror.c >>>> index 86de458..cfef7e9 100644 >>>> --- a/block/mirror.c >>>> +++ b/block/mirror.c >>>> @@ -295,7 +295,7 @@ static void coroutine_fn mirror_run(void *opaque) >>>> BlockDriverState *bs = s->common.bs; >>>> int64_t sector_num, end, sectors_per_chunk, length; >>>> uint64_t last_pause_ns; >>>> - BlockDriverInfo bdi; >>>> + BlockDriverInfo bdi = { .format_specific = NULL }; >>>> char backing_filename[1024]; >>>> int ret = 0; >>>> int n; >>>> @@ -325,6 +325,7 @@ static void coroutine_fn mirror_run(void *opaque) >>>> s->buf_size = MAX(s->buf_size, bdi.cluster_size); >>>> s->cow_bitmap = bitmap_new(length); >>>> } >>>> + qapi_free_ImageInfoSpecific(bdi.format_specific); >>>> } >>>> end = s->common.len >> BDRV_SECTOR_BITS; >>>> @@ -544,13 +545,14 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, >>>> if (granularity == 0) { >>>> /* Choose the default granularity based on the target file's cluster >>>> * size, clamped between 4k and 64k. */ >>>> - BlockDriverInfo bdi; >>>> + BlockDriverInfo bdi = { .format_specific = NULL }; >>>> if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) { >>>> granularity = MAX(4096, bdi.cluster_size); >>>> granularity = MIN(65536, granularity); >>>> } else { >>>> granularity = 65536; >>>> } >>>> + qapi_free_ImageInfoSpecific(bdi.format_specific); >>>> } >>>> assert ((granularity & (granularity - 1)) == 0); >>>> diff --git a/block/qapi.c b/block/qapi.c >>>> index a4bc411..f13fbd5 100644 >>>> --- a/block/qapi.c >>>> +++ b/block/qapi.c >>>> @@ -110,7 +110,7 @@ void bdrv_query_image_info(BlockDriverState *bs, >>>> uint64_t total_sectors; >>>> const char *backing_filename; >>>> char backing_filename2[1024]; >>>> - BlockDriverInfo bdi; >>>> + BlockDriverInfo bdi = { .format_specific = NULL }; >>>> int ret; >>>> Error *err = NULL; >>>> ImageInfo *info = g_new0(ImageInfo, 1); >>>> @@ -133,6 +133,10 @@ void bdrv_query_image_info(BlockDriverState *bs, >>>> } >>>> info->dirty_flag = bdi.is_dirty; >>>> info->has_dirty_flag = true; >>>> + if (bdi.format_specific) { >>>> + info->format_specific = bdi.format_specific; >>>> + info->has_format_specific = true; >>>> + } >>>> } >>>> backing_filename = bs->backing_file; >>>> if (backing_filename[0] != '\0') { >>>> diff --git a/include/block/block.h b/include/block/block.h >>>> index e6b391c..85e9703 100644 >>>> --- a/include/block/block.h >>>> +++ b/include/block/block.h >>>> @@ -18,6 +18,8 @@ typedef struct BlockDriverInfo { >>>> /* offset at which the VM state can be saved (0 if not possible) */ >>>> int64_t vm_state_offset; >>>> bool is_dirty; >>>> + /* additional information; NULL if none */ >>>> + ImageInfoSpecific *format_specific; >>>> } BlockDriverInfo; >>>> typedef struct BlockFragInfo { >>>> diff --git a/qemu-img.c b/qemu-img.c >>>> index b9a848d..ec1ecca 100644 >>>> --- a/qemu-img.c >>>> +++ b/qemu-img.c >>>> @@ -1125,7 +1125,7 @@ static int img_convert(int argc, char **argv) >>>> uint64_t bs_sectors; >>>> uint8_t * buf = NULL; >>>> const uint8_t *buf1; >>>> - BlockDriverInfo bdi; >>>> + BlockDriverInfo bdi = { .format_specific = NULL }; >>>> QEMUOptionParameter *param = NULL, *create_options = NULL; >>>> QEMUOptionParameter *out_baseimg_param; >>>> char *options = NULL; >>>> @@ -1369,6 +1369,7 @@ static int img_convert(int argc, char **argv) >>>> error_report("could not get block driver info"); >>>> goto out; >>>> } >>>> + qapi_free_ImageInfoSpecific(bdi.format_specific); >>>> cluster_size = bdi.cluster_size; >>>> if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) { >>>> error_report("invalid cluster size"); >>>> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c >>>> index f91b6c4..563dd40 100644 >>>> --- a/qemu-io-cmds.c >>>> +++ b/qemu-io-cmds.c >>>> @@ -1677,7 +1677,7 @@ static const cmdinfo_t length_cmd = { >>>> static int info_f(BlockDriverState *bs, int argc, char **argv) >>>> { >>>> - BlockDriverInfo bdi; >>>> + BlockDriverInfo bdi = { .format_specific = NULL }; >>>> char s1[64], s2[64]; >>>> int ret; >>>> @@ -1699,6 +1699,10 @@ static int info_f(BlockDriverState *bs, int argc, char **argv) >>>> printf("cluster size: %s\n", s1); >>>> printf("vm state offset: %s\n", s2); >>>> + if (bdi.format_specific) { >>>> + qapi_free_ImageInfoSpecific(bdi.format_specific); >>>> + } >>>> + >>>> return 0; >>>> } >>>> -- >>>> 1.8.3.1 >>>> >>>> >> Max Max
On Tue, 09/10 09:45, Max Reitz wrote: > On 2013-09-10 09:37, Fam Zheng wrote: > >On Tue, 09/10 09:22, Max Reitz wrote: > >>On 2013-09-10 05:26, Fam Zheng wrote: > >>>On Fri, 09/06 15:12, Max Reitz wrote: > >>>>Add the new ImageInfoSpecific type also to BlockDriverInfo. > >>>> > >>>>To prevent memory leaks, this field has to be initialized to NULL every > >>>>time before calling bdrv_get_info and qapi_free_ImageInfoSpecific has to > >>>>be called on it when the BlockDriverInfo object is no longer required. > >>>> > >>>I don't understand here. I think bdi is always passed into bdrv_get_info() > >>>uninitialized and in bdrv_get_info() there is: > >>> > >>> memset(bdi, 0, sizeof(*bdi)); > >>> > >>>before passing it on to driver, so it's always set to NULL. > >>Oh, you're right, I missed that. Thanks! > >> > >>>And why pointer, not a member to save a free() call? > >>As far as I understand it (and if I don't miss anything again), > >>ImageInfoSpecific is a auto-generated QAPI type, so it may contain > >>pointers to other types anyway (as it does in the case of QCow2, > >>which is the only driver where I have implemented this new field at > >>all; in that case, the compatiblity level is a string), therefore we > >>always need some function to clean up the data referenced by > >>ImageInfoSpecific; qapi_free_ImageInfoSpecific is the perfect one > >>for this, but it takes a heap pointer. > >> > >>Hence, I think using a pointer (to a heap-allocated object) is > >>easier in this case, since the QAPI clean-up function assumes this > >>case. > >> > >OK, learning this from you. Since this is allocated in bdrv_get_info() (from > >the caller PoV), and, the info requires releasing after use, a bdrv_put_info() > >may be a good function to do it, instead of directly operate on the field > >everywhere. > You mean a function for filling the ImageInfoSpecific field? Hm, we > can't really use parameters, since every driver would then require > its own function (which, imo, would defeat the purpose); the only > thing I can imagine right now is a function which converts a JSON > description to the object; however, this would again require proper > escaping for strings and conversion to strings for non-string types, > so I doubt whether this would really help... > No, I mean freeing it. Maybe I should suggest bdrv_free_info() or something else. It's just that too many qapi_free_ImageInfoSpecific(bdi.format_specific); lines after all the bdrv_get_info() calls don't look very clean. > >>>>Signed-off-by: Max Reitz <mreitz@redhat.com> > >>>>--- > >>>> block.c | 3 ++- > >>>> block/mirror.c | 6 ++++-- > >>>> block/qapi.c | 6 +++++- > >>>> include/block/block.h | 2 ++ > >>>> qemu-img.c | 3 ++- > >>>> qemu-io-cmds.c | 6 +++++- > >>>> 6 files changed, 20 insertions(+), 6 deletions(-) > >>>> > >>>>diff --git a/block.c b/block.c > >>>>index 26639e8..1a5d2a4 100644 > >>>>--- a/block.c > >>>>+++ b/block.c > >>>>@@ -1921,7 +1921,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, > >>>> int64_t *cluster_sector_num, > >>>> int *cluster_nb_sectors) > >>>> { > >>>>- BlockDriverInfo bdi; > >>>>+ BlockDriverInfo bdi = { .format_specific = NULL }; > >>>> if (bdrv_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) { > >>>> *cluster_sector_num = sector_num; > >>>>@@ -1932,6 +1932,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, > >>>> *cluster_nb_sectors = QEMU_ALIGN_UP(sector_num - *cluster_sector_num + > >>>> nb_sectors, c); > >>>> } > >>>>+ qapi_free_ImageInfoSpecific(bdi.format_specific); > >>>> } > >>>> static bool tracked_request_overlaps(BdrvTrackedRequest *req, > >>>>diff --git a/block/mirror.c b/block/mirror.c > >>>>index 86de458..cfef7e9 100644 > >>>>--- a/block/mirror.c > >>>>+++ b/block/mirror.c > >>>>@@ -295,7 +295,7 @@ static void coroutine_fn mirror_run(void *opaque) > >>>> BlockDriverState *bs = s->common.bs; > >>>> int64_t sector_num, end, sectors_per_chunk, length; > >>>> uint64_t last_pause_ns; > >>>>- BlockDriverInfo bdi; > >>>>+ BlockDriverInfo bdi = { .format_specific = NULL }; > >>>> char backing_filename[1024]; > >>>> int ret = 0; > >>>> int n; > >>>>@@ -325,6 +325,7 @@ static void coroutine_fn mirror_run(void *opaque) > >>>> s->buf_size = MAX(s->buf_size, bdi.cluster_size); > >>>> s->cow_bitmap = bitmap_new(length); > >>>> } > >>>>+ qapi_free_ImageInfoSpecific(bdi.format_specific); > >>>> } > >>>> end = s->common.len >> BDRV_SECTOR_BITS; > >>>>@@ -544,13 +545,14 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, > >>>> if (granularity == 0) { > >>>> /* Choose the default granularity based on the target file's cluster > >>>> * size, clamped between 4k and 64k. */ > >>>>- BlockDriverInfo bdi; > >>>>+ BlockDriverInfo bdi = { .format_specific = NULL }; > >>>> if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) { > >>>> granularity = MAX(4096, bdi.cluster_size); > >>>> granularity = MIN(65536, granularity); > >>>> } else { > >>>> granularity = 65536; > >>>> } > >>>>+ qapi_free_ImageInfoSpecific(bdi.format_specific); > >>>> } > >>>> assert ((granularity & (granularity - 1)) == 0); > >>>>diff --git a/block/qapi.c b/block/qapi.c > >>>>index a4bc411..f13fbd5 100644 > >>>>--- a/block/qapi.c > >>>>+++ b/block/qapi.c > >>>>@@ -110,7 +110,7 @@ void bdrv_query_image_info(BlockDriverState *bs, > >>>> uint64_t total_sectors; > >>>> const char *backing_filename; > >>>> char backing_filename2[1024]; > >>>>- BlockDriverInfo bdi; > >>>>+ BlockDriverInfo bdi = { .format_specific = NULL }; > >>>> int ret; > >>>> Error *err = NULL; > >>>> ImageInfo *info = g_new0(ImageInfo, 1); > >>>>@@ -133,6 +133,10 @@ void bdrv_query_image_info(BlockDriverState *bs, > >>>> } > >>>> info->dirty_flag = bdi.is_dirty; > >>>> info->has_dirty_flag = true; > >>>>+ if (bdi.format_specific) { > >>>>+ info->format_specific = bdi.format_specific; > >>>>+ info->has_format_specific = true; > >>>>+ } > >>>> } > >>>> backing_filename = bs->backing_file; > >>>> if (backing_filename[0] != '\0') { > >>>>diff --git a/include/block/block.h b/include/block/block.h > >>>>index e6b391c..85e9703 100644 > >>>>--- a/include/block/block.h > >>>>+++ b/include/block/block.h > >>>>@@ -18,6 +18,8 @@ typedef struct BlockDriverInfo { > >>>> /* offset at which the VM state can be saved (0 if not possible) */ > >>>> int64_t vm_state_offset; > >>>> bool is_dirty; > >>>>+ /* additional information; NULL if none */ > >>>>+ ImageInfoSpecific *format_specific; > >>>> } BlockDriverInfo; > >>>> typedef struct BlockFragInfo { > >>>>diff --git a/qemu-img.c b/qemu-img.c > >>>>index b9a848d..ec1ecca 100644 > >>>>--- a/qemu-img.c > >>>>+++ b/qemu-img.c > >>>>@@ -1125,7 +1125,7 @@ static int img_convert(int argc, char **argv) > >>>> uint64_t bs_sectors; > >>>> uint8_t * buf = NULL; > >>>> const uint8_t *buf1; > >>>>- BlockDriverInfo bdi; > >>>>+ BlockDriverInfo bdi = { .format_specific = NULL }; > >>>> QEMUOptionParameter *param = NULL, *create_options = NULL; > >>>> QEMUOptionParameter *out_baseimg_param; > >>>> char *options = NULL; > >>>>@@ -1369,6 +1369,7 @@ static int img_convert(int argc, char **argv) > >>>> error_report("could not get block driver info"); > >>>> goto out; > >>>> } > >>>>+ qapi_free_ImageInfoSpecific(bdi.format_specific); > >>>> cluster_size = bdi.cluster_size; > >>>> if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) { > >>>> error_report("invalid cluster size"); > >>>>diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > >>>>index f91b6c4..563dd40 100644 > >>>>--- a/qemu-io-cmds.c > >>>>+++ b/qemu-io-cmds.c > >>>>@@ -1677,7 +1677,7 @@ static const cmdinfo_t length_cmd = { > >>>> static int info_f(BlockDriverState *bs, int argc, char **argv) > >>>> { > >>>>- BlockDriverInfo bdi; > >>>>+ BlockDriverInfo bdi = { .format_specific = NULL }; > >>>> char s1[64], s2[64]; > >>>> int ret; > >>>>@@ -1699,6 +1699,10 @@ static int info_f(BlockDriverState *bs, int argc, char **argv) > >>>> printf("cluster size: %s\n", s1); > >>>> printf("vm state offset: %s\n", s2); > >>>>+ if (bdi.format_specific) { > >>>>+ qapi_free_ImageInfoSpecific(bdi.format_specific); > >>>>+ } > >>>>+ > >>>> return 0; > >>>> } > >>>>-- > >>>>1.8.3.1 > >>>> > >>>> > >>Max > > Max
On 2013-09-10 09:50, Fam Zheng wrote: > On Tue, 09/10 09:45, Max Reitz wrote: >> On 2013-09-10 09:37, Fam Zheng wrote: >>> On Tue, 09/10 09:22, Max Reitz wrote: >>>> On 2013-09-10 05:26, Fam Zheng wrote: >>>>> On Fri, 09/06 15:12, Max Reitz wrote: >>>>>> Add the new ImageInfoSpecific type also to BlockDriverInfo. >>>>>> >>>>>> To prevent memory leaks, this field has to be initialized to NULL every >>>>>> time before calling bdrv_get_info and qapi_free_ImageInfoSpecific has to >>>>>> be called on it when the BlockDriverInfo object is no longer required. >>>>>> >>>>> I don't understand here. I think bdi is always passed into bdrv_get_info() >>>>> uninitialized and in bdrv_get_info() there is: >>>>> >>>>> memset(bdi, 0, sizeof(*bdi)); >>>>> >>>>> before passing it on to driver, so it's always set to NULL. >>>> Oh, you're right, I missed that. Thanks! >>>> >>>>> And why pointer, not a member to save a free() call? >>>> As far as I understand it (and if I don't miss anything again), >>>> ImageInfoSpecific is a auto-generated QAPI type, so it may contain >>>> pointers to other types anyway (as it does in the case of QCow2, >>>> which is the only driver where I have implemented this new field at >>>> all; in that case, the compatiblity level is a string), therefore we >>>> always need some function to clean up the data referenced by >>>> ImageInfoSpecific; qapi_free_ImageInfoSpecific is the perfect one >>>> for this, but it takes a heap pointer. >>>> >>>> Hence, I think using a pointer (to a heap-allocated object) is >>>> easier in this case, since the QAPI clean-up function assumes this >>>> case. >>>> >>> OK, learning this from you. Since this is allocated in bdrv_get_info() (from >>> the caller PoV), and, the info requires releasing after use, a bdrv_put_info() >>> may be a good function to do it, instead of directly operate on the field >>> everywhere. >> You mean a function for filling the ImageInfoSpecific field? Hm, we >> can't really use parameters, since every driver would then require >> its own function (which, imo, would defeat the purpose); the only >> thing I can imagine right now is a function which converts a JSON >> description to the object; however, this would again require proper >> escaping for strings and conversion to strings for non-string types, >> so I doubt whether this would really help... >> > No, I mean freeing it. Maybe I should suggest bdrv_free_info() or something > else. It's just that too many > > qapi_free_ImageInfoSpecific(bdi.format_specific); > > lines after all the bdrv_get_info() calls don't look very clean. Sounds nice. I'll do it; thanks for the suggestion. >>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>>>> --- >>>>>> block.c | 3 ++- >>>>>> block/mirror.c | 6 ++++-- >>>>>> block/qapi.c | 6 +++++- >>>>>> include/block/block.h | 2 ++ >>>>>> qemu-img.c | 3 ++- >>>>>> qemu-io-cmds.c | 6 +++++- >>>>>> 6 files changed, 20 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/block.c b/block.c >>>>>> index 26639e8..1a5d2a4 100644 >>>>>> --- a/block.c >>>>>> +++ b/block.c >>>>>> @@ -1921,7 +1921,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, >>>>>> int64_t *cluster_sector_num, >>>>>> int *cluster_nb_sectors) >>>>>> { >>>>>> - BlockDriverInfo bdi; >>>>>> + BlockDriverInfo bdi = { .format_specific = NULL }; >>>>>> if (bdrv_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) { >>>>>> *cluster_sector_num = sector_num; >>>>>> @@ -1932,6 +1932,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, >>>>>> *cluster_nb_sectors = QEMU_ALIGN_UP(sector_num - *cluster_sector_num + >>>>>> nb_sectors, c); >>>>>> } >>>>>> + qapi_free_ImageInfoSpecific(bdi.format_specific); >>>>>> } >>>>>> static bool tracked_request_overlaps(BdrvTrackedRequest *req, >>>>>> diff --git a/block/mirror.c b/block/mirror.c >>>>>> index 86de458..cfef7e9 100644 >>>>>> --- a/block/mirror.c >>>>>> +++ b/block/mirror.c >>>>>> @@ -295,7 +295,7 @@ static void coroutine_fn mirror_run(void *opaque) >>>>>> BlockDriverState *bs = s->common.bs; >>>>>> int64_t sector_num, end, sectors_per_chunk, length; >>>>>> uint64_t last_pause_ns; >>>>>> - BlockDriverInfo bdi; >>>>>> + BlockDriverInfo bdi = { .format_specific = NULL }; >>>>>> char backing_filename[1024]; >>>>>> int ret = 0; >>>>>> int n; >>>>>> @@ -325,6 +325,7 @@ static void coroutine_fn mirror_run(void *opaque) >>>>>> s->buf_size = MAX(s->buf_size, bdi.cluster_size); >>>>>> s->cow_bitmap = bitmap_new(length); >>>>>> } >>>>>> + qapi_free_ImageInfoSpecific(bdi.format_specific); >>>>>> } >>>>>> end = s->common.len >> BDRV_SECTOR_BITS; >>>>>> @@ -544,13 +545,14 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, >>>>>> if (granularity == 0) { >>>>>> /* Choose the default granularity based on the target file's cluster >>>>>> * size, clamped between 4k and 64k. */ >>>>>> - BlockDriverInfo bdi; >>>>>> + BlockDriverInfo bdi = { .format_specific = NULL }; >>>>>> if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) { >>>>>> granularity = MAX(4096, bdi.cluster_size); >>>>>> granularity = MIN(65536, granularity); >>>>>> } else { >>>>>> granularity = 65536; >>>>>> } >>>>>> + qapi_free_ImageInfoSpecific(bdi.format_specific); >>>>>> } >>>>>> assert ((granularity & (granularity - 1)) == 0); >>>>>> diff --git a/block/qapi.c b/block/qapi.c >>>>>> index a4bc411..f13fbd5 100644 >>>>>> --- a/block/qapi.c >>>>>> +++ b/block/qapi.c >>>>>> @@ -110,7 +110,7 @@ void bdrv_query_image_info(BlockDriverState *bs, >>>>>> uint64_t total_sectors; >>>>>> const char *backing_filename; >>>>>> char backing_filename2[1024]; >>>>>> - BlockDriverInfo bdi; >>>>>> + BlockDriverInfo bdi = { .format_specific = NULL }; >>>>>> int ret; >>>>>> Error *err = NULL; >>>>>> ImageInfo *info = g_new0(ImageInfo, 1); >>>>>> @@ -133,6 +133,10 @@ void bdrv_query_image_info(BlockDriverState *bs, >>>>>> } >>>>>> info->dirty_flag = bdi.is_dirty; >>>>>> info->has_dirty_flag = true; >>>>>> + if (bdi.format_specific) { >>>>>> + info->format_specific = bdi.format_specific; >>>>>> + info->has_format_specific = true; >>>>>> + } >>>>>> } >>>>>> backing_filename = bs->backing_file; >>>>>> if (backing_filename[0] != '\0') { >>>>>> diff --git a/include/block/block.h b/include/block/block.h >>>>>> index e6b391c..85e9703 100644 >>>>>> --- a/include/block/block.h >>>>>> +++ b/include/block/block.h >>>>>> @@ -18,6 +18,8 @@ typedef struct BlockDriverInfo { >>>>>> /* offset at which the VM state can be saved (0 if not possible) */ >>>>>> int64_t vm_state_offset; >>>>>> bool is_dirty; >>>>>> + /* additional information; NULL if none */ >>>>>> + ImageInfoSpecific *format_specific; >>>>>> } BlockDriverInfo; >>>>>> typedef struct BlockFragInfo { >>>>>> diff --git a/qemu-img.c b/qemu-img.c >>>>>> index b9a848d..ec1ecca 100644 >>>>>> --- a/qemu-img.c >>>>>> +++ b/qemu-img.c >>>>>> @@ -1125,7 +1125,7 @@ static int img_convert(int argc, char **argv) >>>>>> uint64_t bs_sectors; >>>>>> uint8_t * buf = NULL; >>>>>> const uint8_t *buf1; >>>>>> - BlockDriverInfo bdi; >>>>>> + BlockDriverInfo bdi = { .format_specific = NULL }; >>>>>> QEMUOptionParameter *param = NULL, *create_options = NULL; >>>>>> QEMUOptionParameter *out_baseimg_param; >>>>>> char *options = NULL; >>>>>> @@ -1369,6 +1369,7 @@ static int img_convert(int argc, char **argv) >>>>>> error_report("could not get block driver info"); >>>>>> goto out; >>>>>> } >>>>>> + qapi_free_ImageInfoSpecific(bdi.format_specific); >>>>>> cluster_size = bdi.cluster_size; >>>>>> if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) { >>>>>> error_report("invalid cluster size"); >>>>>> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c >>>>>> index f91b6c4..563dd40 100644 >>>>>> --- a/qemu-io-cmds.c >>>>>> +++ b/qemu-io-cmds.c >>>>>> @@ -1677,7 +1677,7 @@ static const cmdinfo_t length_cmd = { >>>>>> static int info_f(BlockDriverState *bs, int argc, char **argv) >>>>>> { >>>>>> - BlockDriverInfo bdi; >>>>>> + BlockDriverInfo bdi = { .format_specific = NULL }; >>>>>> char s1[64], s2[64]; >>>>>> int ret; >>>>>> @@ -1699,6 +1699,10 @@ static int info_f(BlockDriverState *bs, int argc, char **argv) >>>>>> printf("cluster size: %s\n", s1); >>>>>> printf("vm state offset: %s\n", s2); >>>>>> + if (bdi.format_specific) { >>>>>> + qapi_free_ImageInfoSpecific(bdi.format_specific); >>>>>> + } >>>>>> + >>>>>> return 0; >>>>>> } >>>>>> -- >>>>>> 1.8.3.1 >>>>>> >>>>>> >>>> Max >> Max Max
diff --git a/block.c b/block.c index 26639e8..1a5d2a4 100644 --- a/block.c +++ b/block.c @@ -1921,7 +1921,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, int64_t *cluster_sector_num, int *cluster_nb_sectors) { - BlockDriverInfo bdi; + BlockDriverInfo bdi = { .format_specific = NULL }; if (bdrv_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) { *cluster_sector_num = sector_num; @@ -1932,6 +1932,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, *cluster_nb_sectors = QEMU_ALIGN_UP(sector_num - *cluster_sector_num + nb_sectors, c); } + qapi_free_ImageInfoSpecific(bdi.format_specific); } static bool tracked_request_overlaps(BdrvTrackedRequest *req, diff --git a/block/mirror.c b/block/mirror.c index 86de458..cfef7e9 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -295,7 +295,7 @@ static void coroutine_fn mirror_run(void *opaque) BlockDriverState *bs = s->common.bs; int64_t sector_num, end, sectors_per_chunk, length; uint64_t last_pause_ns; - BlockDriverInfo bdi; + BlockDriverInfo bdi = { .format_specific = NULL }; char backing_filename[1024]; int ret = 0; int n; @@ -325,6 +325,7 @@ static void coroutine_fn mirror_run(void *opaque) s->buf_size = MAX(s->buf_size, bdi.cluster_size); s->cow_bitmap = bitmap_new(length); } + qapi_free_ImageInfoSpecific(bdi.format_specific); } end = s->common.len >> BDRV_SECTOR_BITS; @@ -544,13 +545,14 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, if (granularity == 0) { /* Choose the default granularity based on the target file's cluster * size, clamped between 4k and 64k. */ - BlockDriverInfo bdi; + BlockDriverInfo bdi = { .format_specific = NULL }; if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) { granularity = MAX(4096, bdi.cluster_size); granularity = MIN(65536, granularity); } else { granularity = 65536; } + qapi_free_ImageInfoSpecific(bdi.format_specific); } assert ((granularity & (granularity - 1)) == 0); diff --git a/block/qapi.c b/block/qapi.c index a4bc411..f13fbd5 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -110,7 +110,7 @@ void bdrv_query_image_info(BlockDriverState *bs, uint64_t total_sectors; const char *backing_filename; char backing_filename2[1024]; - BlockDriverInfo bdi; + BlockDriverInfo bdi = { .format_specific = NULL }; int ret; Error *err = NULL; ImageInfo *info = g_new0(ImageInfo, 1); @@ -133,6 +133,10 @@ void bdrv_query_image_info(BlockDriverState *bs, } info->dirty_flag = bdi.is_dirty; info->has_dirty_flag = true; + if (bdi.format_specific) { + info->format_specific = bdi.format_specific; + info->has_format_specific = true; + } } backing_filename = bs->backing_file; if (backing_filename[0] != '\0') { diff --git a/include/block/block.h b/include/block/block.h index e6b391c..85e9703 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -18,6 +18,8 @@ typedef struct BlockDriverInfo { /* offset at which the VM state can be saved (0 if not possible) */ int64_t vm_state_offset; bool is_dirty; + /* additional information; NULL if none */ + ImageInfoSpecific *format_specific; } BlockDriverInfo; typedef struct BlockFragInfo { diff --git a/qemu-img.c b/qemu-img.c index b9a848d..ec1ecca 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1125,7 +1125,7 @@ static int img_convert(int argc, char **argv) uint64_t bs_sectors; uint8_t * buf = NULL; const uint8_t *buf1; - BlockDriverInfo bdi; + BlockDriverInfo bdi = { .format_specific = NULL }; QEMUOptionParameter *param = NULL, *create_options = NULL; QEMUOptionParameter *out_baseimg_param; char *options = NULL; @@ -1369,6 +1369,7 @@ static int img_convert(int argc, char **argv) error_report("could not get block driver info"); goto out; } + qapi_free_ImageInfoSpecific(bdi.format_specific); cluster_size = bdi.cluster_size; if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) { error_report("invalid cluster size"); diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index f91b6c4..563dd40 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -1677,7 +1677,7 @@ static const cmdinfo_t length_cmd = { static int info_f(BlockDriverState *bs, int argc, char **argv) { - BlockDriverInfo bdi; + BlockDriverInfo bdi = { .format_specific = NULL }; char s1[64], s2[64]; int ret; @@ -1699,6 +1699,10 @@ static int info_f(BlockDriverState *bs, int argc, char **argv) printf("cluster size: %s\n", s1); printf("vm state offset: %s\n", s2); + if (bdi.format_specific) { + qapi_free_ImageInfoSpecific(bdi.format_specific); + } + return 0; }
Add the new ImageInfoSpecific type also to BlockDriverInfo. To prevent memory leaks, this field has to be initialized to NULL every time before calling bdrv_get_info and qapi_free_ImageInfoSpecific has to be called on it when the BlockDriverInfo object is no longer required. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 3 ++- block/mirror.c | 6 ++++-- block/qapi.c | 6 +++++- include/block/block.h | 2 ++ qemu-img.c | 3 ++- qemu-io-cmds.c | 6 +++++- 6 files changed, 20 insertions(+), 6 deletions(-)