Re: [Qemu-devel] [PATCH v2 2/6] block: Add ImageInfoSpecific to BlockDriverInfo

[Max Reitz <mreitz@redhat.com>]

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