Re: [Qemu-devel] [PATCH V7 1/4] block/qemu-img: Refine and export infinite loop checking in collect_image_info_list()

[Fam Zheng <famz@redhat.com>]

On 2013年11月13日 10:39, Xu Wang wrote:
> If there is a loop in the backing file chain, it could cause problems
> such as no response or a segfault during system boot. Hence detecting a
> backing file loop is necessary. This patch extracts the loop check from
> collect_image_info_list() in block.c into independent functions
> bdrv_backing_chain_okay() and bdrv_image_create_okay().
>
> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
> ---
>   block.c               | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   include/block/block.h |  3 ++
>   qemu-img.c            | 52 +++++++++++++++++------------------
>   3 files changed, 105 insertions(+), 26 deletions(-)
>
> diff --git a/block.c b/block.c
> index 58efb5b..3c43179 100644
> --- a/block.c
> +++ b/block.c
> @@ -4490,6 +4490,82 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
>       bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
>   }
>
> +static bool file_chain_has_loop(GHashTable *filenames, const char *filename,
> +                                  const char *fmt)
> +{
> +    BlockDriverState *bs;
> +    char fbuf[PATH_MAX];
> +    int ret;
> +    Error *local_err = NULL;
> +
> +    while (filename && (filename[0] != '\0')) {
> +        if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
> +            error_report("Backing file '%s' creates an infinite loop.",
> +                         filename);
> +            return true;
> +        }

I would expect the caller to format error message and report to user. 
But this is acceptable. Let's see if other reviewers have more opinions.

Fam

> +        g_hash_table_insert(filenames, (gpointer)filename, NULL);
> +
> +        bs = bdrv_new("image");
> +
> +        ret = bdrv_open(bs, filename, NULL,
> +                        BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, NULL, &local_err);
> +        if (ret < 0) {
> +            error_report("Could not open '%s': %s", filename,
> +                         error_get_pretty(local_err));
> +            error_free(local_err);
> +            local_err = NULL;
> +            return true;
> +        }
> +
> +        bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf));
> +        filename = fbuf;
> +        fmt = NULL;
> +
> +        bdrv_unref(bs);
> +    }
> +
> +    return false;
> +}
> +
> +/**
> + * Check backing file chain if there is a loop in it.
> + *
> + * @filename: topmost image filename of backing file chain.
> + * @fmt: topmost image format of backing file chain(may be NULL to autodetect).
> + * @new_filename: if a new image to be created and takes @filename as backing
> + *                file, the new chain should be checked before creating.
> + *
> + * Returns: true for backing chain okay, false for loop happened.
> + */
> +bool bdrv_backing_chain_okay(const char *filename, const char *fmt,
> +                             const char *new_filename)
> +{
> +    GHashTable *filenames;
> +
> +    filenames = g_hash_table_new_full(g_str_hash, g_str_equal, NULL, NULL);
> +
> +    if (filename == NULL || filename[0] == '\0') {
> +        goto exit;
> +    }
> +
> +    if (new_filename && new_filename[0] != '\0') {
> +        g_hash_table_insert(filenames, (gpointer)new_filename, NULL);
> +    }
> +
> +    if (file_chain_has_loop(filenames, filename, fmt)) {
> +        goto err;
> +    }
> +
> +exit:
> +    g_hash_table_destroy(filenames);
> +    return true;
> +
> +err:
> +    g_hash_table_destroy(filenames);
> +    return false;
> +}
> +
>   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 3560deb..7ad714f 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -378,6 +378,9 @@ 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_chain_okay(const char *filename, const char *fmt,
> +                             const char *new_filename);
> +
>   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 bf3fb4f..f8644c6 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -281,6 +281,14 @@ static BlockDriverState *bdrv_new_open(const char *filename,
>           drv = NULL;
>       }
>
> +    /* check backing file loop if the whole chain need to be opened */
> +    if (!(flags & BDRV_O_NO_BACKING) &&
> +        !bdrv_backing_chain_okay(filename, fmt, NULL)) {
> +        error_report("bdrv_new_open: Open %s failed. An infinite loop exists "
> +                     "in the backing chain", filename);
> +        goto fail;
> +    }
> +
>       ret = bdrv_open(bs, filename, NULL, flags, drv, &local_err);
>       if (ret < 0) {
>           error_report("Could not open '%s': %s", filename,
> @@ -1641,11 +1649,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
>    *
> @@ -1663,30 +1666,24 @@ 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;
> +    int flags = BDRV_O_FLAGS;
>
> -    filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
> -
> -    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);
> +    if (!chain) {
> +        flags |= BDRV_O_NO_BACKING;
> +    }
>
> -        bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING,
> -                           false, false);
> -        if (!bs) {
> -            goto err;
> -        }
> +    bs = bdrv_new_open(filename, fmt, flags,
> +                       false, false);
> +    if (!bs) {
> +        goto err;
> +    }
>
> +    while (filename) {
>           bdrv_query_image_info(bs, &info, &err);
>           if (error_is_set(&err)) {
>               error_report("%s", error_get_pretty(err));
> @@ -1711,14 +1708,17 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>               if (info->has_backing_filename_format) {
>                   fmt = info->backing_filename_format;
>               }
> +
> +            if (filename) {
> +                bs = bdrv_find_backing_image(bs, filename);
> +            }
>           }
>       }
> -    g_hash_table_destroy(filenames);
> +
>       return head;
>
>   err:
>       qapi_free_ImageInfoList(head);
> -    g_hash_table_destroy(filenames);
>       return NULL;
>   }
>
>