From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40725) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJIHd-0006yV-VT for qemu-devel@nongnu.org; Tue, 10 Sep 2013 03:22:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VJIHX-0006XW-T4 for qemu-devel@nongnu.org; Tue, 10 Sep 2013 03:22:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25740) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJIHX-0006XQ-L3 for qemu-devel@nongnu.org; Tue, 10 Sep 2013 03:22:39 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r8A7Md4r031737 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 10 Sep 2013 03:22:39 -0400 Message-ID: <522EC8BC.40907@redhat.com> Date: Tue, 10 Sep 2013 09:22:36 +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> In-Reply-To: <20130910032629.GA8016@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 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 >> --- >> 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