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

[Xu Wang <cngesaint@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 9591 bytes --]

2013/7/10 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.
>
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
>

[-- Attachment #2: Type: text/html, Size: 13144 bytes --]