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 <mre...@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
>
>
