From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45917) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJIdp-0006Uq-QJ for qemu-devel@nongnu.org; Tue, 10 Sep 2013 03:45:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VJIdj-0005Ei-PA for qemu-devel@nongnu.org; Tue, 10 Sep 2013 03:45:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39541) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJIdj-0005Eb-Gs for qemu-devel@nongnu.org; Tue, 10 Sep 2013 03:45:35 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r8A7jYp0006435 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 10 Sep 2013 03:45:35 -0400 Message-ID: <522ECE1C.6040503@redhat.com> Date: Tue, 10 Sep 2013 09:45:32 +0200 From: Max Reitz MIME-Version: 1.0 References: <1378473154-30057-1-git-send-email-mreitz@redhat.com> <1378473154-30057-3-git-send-email-mreitz@redhat.com> <20130910032629.GA8016@T430s.nay.redhat.com> <522EC8BC.40907@redhat.com> <20130910073724.GB21082@T430s.nay.redhat.com> In-Reply-To: <20130910073724.GB21082@T430s.nay.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/6] block: Add ImageInfoSpecific to BlockDriverInfo List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: famz@redhat.com Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi 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 >>>> --- >>>> 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