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 <mre...@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
