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

[Fam Zheng <famz@redhat.com>]

On Mon, 07/08 03:26, Xu Wang wrote:
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  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.
> + * @chain: true  - enumerate entire backing file chain
> + *         false - only topmost image file
Does it make sense when chain==false for loop check?
> + * @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 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.
> + */
> +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.
> +            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?
> +
> +        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);
> +
>  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.
> +    /* 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