From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47469) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJImI-0004Lc-SQ for qemu-devel@nongnu.org; Tue, 10 Sep 2013 03:54:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VJImC-0007bv-R3 for qemu-devel@nongnu.org; Tue, 10 Sep 2013 03:54:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:8665) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJImC-0007bp-H5 for qemu-devel@nongnu.org; Tue, 10 Sep 2013 03:54:20 -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 r8A7sJ8b010444 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 10 Sep 2013 03:54:20 -0400 Message-ID: <522ED029.3010007@redhat.com> Date: Tue, 10 Sep 2013 09:54:17 +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> <522ECE1C.6040503@redhat.com> <20130910075044.GC21082@T430s.nay.redhat.com> In-Reply-To: <20130910075044.GC21082@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: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 >>>>>> --- >>>>>> 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