Patchwork [v2,2/6] block: Add ImageInfoSpecific to BlockDriverInfo

login
register
mail settings
Submitter Max Reitz
Date Sept. 6, 2013, 1:12 p.m.
Message ID <1378473154-30057-3-git-send-email-mreitz@redhat.com>
Download mbox | patch
Permalink /patch/273190/
State New
Headers show

Comments

Max Reitz - Sept. 6, 2013, 1:12 p.m.
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(-)
Fam Zheng - Sept. 10, 2013, 3:26 a.m.
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
> 
>
Max Reitz - Sept. 10, 2013, 7:22 a.m.
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
Fam Zheng - Sept. 10, 2013, 7:37 a.m.
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
Max Reitz - Sept. 10, 2013, 7:45 a.m.
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
Fam Zheng - Sept. 10, 2013, 7:50 a.m.
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
Max Reitz - Sept. 10, 2013, 7:54 a.m.
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

Patch

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;
 }