Re: [Qemu-devel] [PATCH V2 1/5] Refine and export infinite loop checking

On Mon, 07/08 03:26, Xu Wang wrote:
> Signed-off-by: Xu Wang <address@hidden>
> ---
> block.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 4 ++
> qemu-img.c | 30 +++++----------
> 3 files changed, 114 insertions(+), 21 deletions(-)
>
> diff --git a/block.c b/block.c
> index b88ad2f..53b1a01 100644
> --- a/block.c
> +++ b/block.c
> @@ -4431,6 +4431,107 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
> bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
> }
>
> +static gboolean str_equal_func(gconstpointer a, gconstpointer b)
> +{
> + return strcmp(a, b) == 0;
> +}
> +
> +/**
> + * Check backing file chain if there is a loop in it and build list of
> + * ImageInfo if needed.
> + *
> + * @filename: topmost image filename, absolute or relative path is OK.
> + * @fmt: topmost image format (may be NULL to autodetect)
> + * @head: head of ImageInfo list. If not need please set head to null.

This parameter is removed, you can also remove the comment.

Sorry. I'll update in new version.

> + * @chain: true - enumerate entire backing file chain
> + * false - only topmost image file

Does it make sense when chain==false for loop check?

If chain==false, it means that backing file chain check will be ignored. It just

be used to be compatible with original function variables. However, the caller

calls bdrv_backing_file_loop_check() means that 'hey, you should check backing
files and tell me if there is a loop in it'. So I will remove it and do @chain logic
before call bdrv_backing_file_loop_check() in collect_image_info_list().

> + * @backing_file: if this value is set, @filename inode (-1 if it doesn't
> + * exist) will insert into hash table directly. Loop check
> + * starts from it. Absolute or relative path is OK for it.
> + * @backing_format: format of backing file

Why these two parameters? The caller can still pass bs->backing_file to
filename and bs->backing_fmt to fmt without them, to skip the topmost.

If @filename just a filename to be created, it will couldn't get the backing

file name by calling bdrv_get_backing_file(). So caller could pass them by these
parameters to tell function 'The filename is pointed but it may just a filename, you

couldn't get backing file from it. But I'll give it by parameter'.

> + *
> + * If return true, stands for a backing file loop exists or some error
> + * happened. If return false, everything is ok.

Please format like this:
+ * Returns: true for backing file loop, false for no loop.

OK, I'll update it in new version These are my first patches list for qemu. I will
do my best to learn rules about it as soon as possible. Thank you very much
for the advice from all of yours:-)

> + */
> +bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
> + bool chain, const char *backing_file,
> + const char *backing_format) {
> + GHashTable *inodes;
> + BlockDriverState *bs;
> + BlockDriver *drv;
> + struct stat sbuf;
> + long inode = 0;
> + int ret;
> + char fbuf[1024];
> +
> + inodes = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
> +
> + if (backing_file) {
> + /* Check if file exists. */
> + if (access(filename, F_OK)) {
> + inode = -1;
> + } else {
> + if (stat(filename, &sbuf) == -1) {
> + error_report("Get file %s stat failed.", filename);
> + goto err;
> + }
> + inode = (long)sbuf.st_ino;
> + }
> +
> + filename = backing_file;
> + fmt = backing_format;
> + g_hash_table_insert(inodes, (gpointer)&inode, NULL);
> + }
> +
> + while (filename && (filename[0] != '\0')) {
> + if (stat(filename, &sbuf) == -1) {

Does it mean stat() on backing_file twice if it's not NULL? As you
assigned backing_file to filename above.

> + error_report("Get file %s stat failed.", filename);
> + goto err;
> + }
> + inode = (long)sbuf.st_ino;
> +
> + if (g_hash_table_lookup_extended(inodes, &inode, NULL, NULL)) {
> + error_report("Backing file '%s' creates an infinite loop.",
> + filename);
> + goto err;
> + }
> + g_hash_table_insert(inodes, (gpointer)&inode, NULL);
> +
> + bs = bdrv_new("image");
> +
> + if (fmt) {
> + drv = bdrv_find_format(fmt);
> + if (!drv) {
> + error_report("Unknown file format '%s'", fmt);
> + goto err;

bs is leaked.

> + }
> + } else {
> + drv = NULL;
> + }
> +
> + ret = bdrv_open(bs, filename, NULL,
> + BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv);
> + if (ret < 0) {

bs is leaked.

bdrv_delete will be added in err.

> + error_report("Could not open '%s': %s", filename, strerror(-ret));
> + goto err;
> + }

These lines looks like a copy & paste from bdrv_new_open(). Could you
just call it?

bdrv_new_open() is implemented as a static function in qemu-img.c and qemu-img.c
includes block.c (). If bdrv_backing_file_loop_check() calls bdrv_new_open(),

It's really a bit strange. If it's needed to make bdrv_new_open() moved to block.c,

I'll do work for it.

> +
> + filename = fmt = NULL;
> + if (chain) {
> + bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf));
> + filename = fbuf;
> + }
> +
> + bdrv_delete(bs);
> + }
> + g_hash_table_destroy(inodes);
> + return false;
> +
> +err:
> + g_hash_table_destroy(inodes);
> + return true;
> +}
> +
> void bdrv_img_create(const char *filename, const char *fmt,
> const char *base_filename, const char *base_fmt,
> char *options, uint64_t img_size, int flags,
> diff --git a/include/block/block.h b/include/block/block.h
> index 2307f67..6c631f2 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -332,6 +332,10 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
> int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
> int64_t pos, int size);
>
> +bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
> + bool chain, const char *backing_file,
> + const char *backing_format);

Please align to parameter beginning:

bool chain, const char *backing_file,

const char *backing_format);

Sorry for that.

> +
> void bdrv_img_create(const char *filename, const char *fmt,
> const char *base_filename, const char *base_fmt,
> char *options, uint64_t img_size, int flags,
> diff --git a/qemu-img.c b/qemu-img.c
> index 809b4f1..48284c5 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1620,11 +1620,6 @@ static void dump_human_image_info_list(ImageInfoList *list)
> }
> }
>
> -static gboolean str_equal_func(gconstpointer a, gconstpointer b)
> -{
> - return strcmp(a, b) == 0;
> -}
> -
> /**
> * Open an image file chain and return an ImageInfoList
> *
> @@ -1642,24 +1637,18 @@ static ImageInfoList *collect_image_info_list(const char *filename,
> bool chain)
> {
> ImageInfoList *head = NULL;
> + BlockDriverState *bs;
> + ImageInfoList *elem;
> ImageInfoList **last = &head;
> - GHashTable *filenames;
> + ImageInfo *info;
> Error *err = NULL;
> -
> - filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
> +

Please avoid trailing whitespaces. You could use scripts/checkpatch.pl
to check your patch for coding style problems.

OK, I'll do that.

Xu Wang

> + /* check if loop exists and build ImageInfoList */
> + if (bdrv_backing_file_loop_check(filename, fmt, chain, NULL, NULL)) {
> + goto err;
> + }
>
> while (filename) {
> - BlockDriverState *bs;
> - ImageInfo *info;
> - ImageInfoList *elem;
> -
> - if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
> - error_report("Backing file '%s' creates an infinite loop.",
> - filename);
> - goto err;
> - }
> - g_hash_table_insert(filenames, (gpointer)filename, NULL);
> -
> bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING,
> false, false);
> if (!bs) {
> @@ -1692,12 +1681,11 @@ static ImageInfoList *collect_image_info_list(const char *filename,
> }
> }
> }
> - g_hash_table_destroy(filenames);
> +
> return head;
>
> err:
> qapi_free_ImageInfoList(head);
> - g_hash_table_destroy(filenames);
> return NULL;
> }
>
> --
> 1.8.1.4
>
>

--
Fam

From:	Xu Wang
Subject:	Re: [Qemu-devel] [PATCH V2 1/5] Refine and export infinite loop checking in collect_image_info_list()
Date:	Fri, 12 Jul 2013 10:58:15 +0800