[Qemu-devel] [PATCH V6 0/5] Refine and export backing file loop check

[Qemu-devel] [PATCH V6 0/5] Refine and export backing file loop check

[Xu Wang]

If there is loop exists in the backing file chain, many problems
could be caused by it, such as no response and segment fault during
system boot. Hence stopping backing file loop appear is very necessary.
These patches refine and export loop checking function from collect_image_
info_list() to block.c and build a independent function named bdrv_
backing_file_loop_check(). Backing file loop checking is added before
image created, before change backing file and before system boot.

Updates from V5:
  1. Simplify the function of loop checking (Just filename comparation.
     Thanks Eric's suggestion).
  2. Delete WIN32 platform support (There is no need to this patch now).
  3. Adjust position of backing file loop checking (calling checking function
     before change happen).
  4. Function name updates and comments description fix.

Updates from V4:
  1. Add backing file loop check in bdrv_new_open().
  2. Adjust open file logic of collect_image_info_list() (bdrv_new_open()
     is called only once when opening the whole chain).
  3. Remove redundant brackets in lnk file check logic.
  4. Add error output in bdrv_img_create().
  5. Remove MAX_PATH_LEN to use PATH_MAX instead.

Updates from V3:
  1. Comments fix for function bdrv_backing_file_loop_check().
  2. Add ret check for fseek()/fread() in get_lnk_target_file().
  3. Add limit of shortcuts filename length reading during comparing.
  4. Add error_report() in driv_init().
  5. Remove redundant loop check in qcow2/qed_change_backing_file().

Updates from V2:
  1. Removed parameter @chain from bdrv_backing_file_loop_check()
  2. Comments and format fix, all patches were checked by checkpatch.pl
  3. Fixed *bs leak.
  4. Improved logic of .lnk file recognization.
  5. Add filename lenth limit check in while()
  6. Changed get_win_inode() to get_inode() and move all inode get method
     into it to make logic more simpler.
  7. Added value of @fmt as suggested.
  8. Added backing file loop check in qcow2.c/qed.c

Xu Wang (5):
  block/qemu-img: Refine and export infinite loop checking in
    collect_image_info_list()
  qemu-img: Add infinite loop checking in bdrv_new_open()
  block: Add check infinite loop in bdrv_img_create()
  block: Add backing file loop check in change_backing_file()
  blockdev: Add infinite loop check in drive_init()

 block.c               | 130 ++++++++++++++++++++++++++++++++++++++++++++++++--
 blockdev.c            |   6 +++
 include/block/block.h |   4 ++
 qemu-img.c            |  52 ++++++++++----------
 4 files changed, 162 insertions(+), 30 deletions(-)

-- 
1.8.1.4

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

[Xu Wang]

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               | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |   4 ++
 qemu-img.c            |  44 ++++++++-----------
 3 files changed, 139 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index 58efb5b..3443117 100644
--- a/block.c
+++ b/block.c
@@ -4490,6 +4490,123 @@ 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;
+}
+
+static bool file_chain_loop_check(GHashTable *filenames, const char *filename,
+                                  const char *fmt) {
+    BlockDriverState *bs;
+    BlockDriver *drv;
+    char fbuf[1024];
+    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;
+        }
+        g_hash_table_insert(filenames, (gpointer)filename, NULL);
+
+        bs = bdrv_new("image");
+
+        if (fmt) {
+            drv = bdrv_find_format(fmt);
+            if (!drv) {
+                error_report("Unknown file format '%s'", fmt);
+                bdrv_delete(bs);
+                return true;
+            }
+        } else {
+            drv = NULL;
+        }
+
+        ret = bdrv_open(bs, filename, NULL,
+                        BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv, &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).
+ *
+ * Returns: true for backing file loop or error happened, false for no loop.
+ */
+bool bdrv_backing_chain_okay(const char *filename, const char *fmt) {
+    GHashTable *filenames;
+
+    if (filename == NULL || filename[0] == '\0') {
+        return true;
+    }
+
+    filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
+
+    if (file_chain_loop_check(filenames, filename, fmt)) {
+        goto err;
+    }
+
+    g_hash_table_destroy(filenames);
+    return true;
+
+err:
+    g_hash_table_destroy(filenames);
+    return false;
+}
+
+/**
+ * Check if there is loop exists in the backing file chain and if there will
+ * be loop occur after backing file chain updated or new image created.
+ *
+ * @filename: the image filename to be created.
+ * @backing_file: topmost image filename of backing file chain.
+ * @backing_fmt: topmost image format (may be NULL to autodetect).
+ *
+ * Returns: true for backing file loop or error happened, false for no loop.
+ */
+bool bdrv_new_chain_okay(const char *filename, const char *backing_file,
+                          const char *backing_fmt) {
+    GHashTable *filenames;
+
+    if (backing_file == NULL || backing_file[0] == '\0') {
+        return true;
+    }
+
+    filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
+
+    g_hash_table_insert(filenames, (gpointer)filename, NULL);
+
+    if (file_chain_loop_check(filenames, backing_file, backing_fmt)) {
+        goto err;
+    }
+
+    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..0945c09 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -378,6 +378,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_chain_okay(const char *filename, const char *fmt);
+bool bdrv_new_chain_okay(const char *filename, const char *backing_file,
+                            const char *backing_fmt);
+
 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..d5ec45b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1641,11 +1641,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 +1658,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 +1700,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;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH V6 2/5] qemu-img: Add infinite loop checking in bdrv_new_open()

[Xu Wang]

Every image should be checked if there is infinite loop in backing
file chain before open it. So infinite loop check was added into
bdrv_new_open(). If @filename is opened without the flag
BDRV_O_NO_BACKING, the infinite loop check should be called.

Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
---
 qemu-img.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index d5ec45b..3af7996 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)) {
+        error_report("bdrv_new_open: Open %s failed. There is 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,
-- 
1.8.1.4

[Qemu-devel] [PATCH V6 3/5] block: Add check infinite loop in bdrv_img_create()

[Xu Wang]

Backing file loop should be checked before qemu-img create command
execution. If loop is found, qemu-img create should be stopped and
an error printed.

Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
---
 block.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 3443117..8423e80 100644
--- a/block.c
+++ b/block.c
@@ -4670,15 +4670,15 @@ void bdrv_img_create(const char *filename, const char *fmt,
     }
 
     backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
+    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
     if (backing_file && backing_file->value.s) {
-        if (!strcmp(filename, backing_file->value.s)) {
-            error_setg(errp, "Error: Trying to create an image with the "
-                             "same filename as the backing file");
+        if (!bdrv_new_chain_okay(filename, backing_file->value.s,
+                                backing_fmt->value.s)) {
+            error_report("bdrv_img_create: loop exists, image create failed");
             goto out;
         }
     }
 
-    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
     if (backing_fmt && backing_fmt->value.s) {
         backing_drv = bdrv_find_format(backing_fmt->value.s);
         if (!backing_drv) {
-- 
1.8.1.4

[Qemu-devel] [PATCH V6 4/5] block: Add backing file loop check in change_backing_file()

[Xu Wang]

Backing file loop should be checked before calling change_backing_
file(). If loop appeared, this calling should be stopped and an
error printed.

Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
---
 block.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block.c b/block.c
index 8423e80..cb50bfd 100644
--- a/block.c
+++ b/block.c
@@ -2083,6 +2083,11 @@ int bdrv_change_backing_file(BlockDriverState *bs,
         return -EINVAL;
     }
 
+    /* Check if loop exists in backing files chain after change */
+    if (!bdrv_new_chain_okay(bs->filename, backing_file, backing_fmt)) {
+        return -EIO;
+    }
+
     if (drv->bdrv_change_backing_file != NULL) {
         ret = drv->bdrv_change_backing_file(bs, backing_file, backing_fmt);
     } else {
-- 
1.8.1.4

[Qemu-devel] [PATCH V6 5/5] blockdev: Add infinite loop check in drive_init()

[Xu Wang]

Check the backing file for a loop during image boot, to avoid a lack or
response or segfault.

Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
---
 blockdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index b260477..7c0927f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -510,6 +510,12 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
 
     bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
 
+    /* Add backing file loop check */
+    if (!bdrv_backing_chain_okay(file, drv ? drv->format_name : NULL)) {
+        error_report("drive_init: backing file loop check failed");
+        goto err;
+    }
+
     QINCREF(bs_opts);
     ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
 
-- 
1.8.1.4

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

[Fam Zheng]

On Tue, 11/05 22:09, 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               | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |   4 ++
>  qemu-img.c            |  44 ++++++++-----------
>  3 files changed, 139 insertions(+), 26 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 58efb5b..3443117 100644
> --- a/block.c
> +++ b/block.c
> @@ -4490,6 +4490,123 @@ 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;
> +}

Just use g_str_equal here.

> +
> +static bool file_chain_loop_check(GHashTable *filenames, const char *filename,
> +                                  const char *fmt) {

Open brace '{' should be in a new line for functions.

Still confusing function name and return type. Suggest file_chain_has_loop().

> +    BlockDriverState *bs;
> +    BlockDriver *drv;
> +    char fbuf[1024];

Could use 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;
> +        }
> +        g_hash_table_insert(filenames, (gpointer)filename, NULL);
> +
> +        bs = bdrv_new("image");
> +
> +        if (fmt) {
> +            drv = bdrv_find_format(fmt);
> +            if (!drv) {
> +                error_report("Unknown file format '%s'", fmt);
> +                bdrv_delete(bs);
> +                return true;
> +            }
> +        } else {
> +            drv = NULL;
> +        }

No need to call bdrv_find_format for multiple times. Also it doesn't look good
to write format checking here, just let the caller pass in drv.

> +
> +        ret = bdrv_open(bs, filename, NULL,
> +                        BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv, &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).
> + *
> + * Returns: true for backing file loop or error happened, false for no loop.

Really?

> + */
> +bool bdrv_backing_chain_okay(const char *filename, const char *fmt) {
> +    GHashTable *filenames;
> +
> +    if (filename == NULL || filename[0] == '\0') {
> +        return true;

Please don't mix "goto err" and multiple "return true". Could be

           goto exit;

...

> +    }
> +
> +    filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
> +
> +    if (file_chain_loop_check(filenames, filename, fmt)) {
> +        goto err;
> +    }
> +
> +    g_hash_table_destroy(filenames);
  exit:

> +    return true;
> +
> +err:
> +    g_hash_table_destroy(filenames);
> +    return false;
> +}
> +
> +/**
> + * Check if there is loop exists in the backing file chain and if there will
> + * be loop occur after backing file chain updated or new image created.
> + *
> + * @filename: the image filename to be created.
> + * @backing_file: topmost image filename of backing file chain.
> + * @backing_fmt: topmost image format (may be NULL to autodetect).
> + *
> + * Returns: true for backing file loop or error happened, false for no loop.

I don't think so.

> + */
> +bool bdrv_new_chain_okay(const char *filename, const char *backing_file,
> +                          const char *backing_fmt) {

Please align arguments:     ^

This function could be merged to bdrv_backing_chain_ok by adding an optional
argument const char *new_filename. If it's not NULL you add it to the hash
table, like:

    bool bdrv_backing_chain_okay(const char *filename, const char *fmt,
                                 const char *new_filename)
    {
        GHashTable *filenames;

        if (filename == NULL || filename[0] == '\0') {
            goto exit;
        }

        filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);

        if (new_filename && new_filename[0] != '\0') {
            g_hash_table_insert(filenames, (gpointer)filename, NULL);
        }

        if (file_chain_loop_check(filenames, filename, fmt)) {
            goto err;
        }

        g_hash_table_destroy(filenames);
    exit:
        return true;

    err:
        g_hash_table_destroy(filenames);
        return false;
    }


> +    GHashTable *filenames;
> +
> +    if (backing_file == NULL || backing_file[0] == '\0') {
> +        return true;
> +    }
> +
> +    filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
> +
> +    g_hash_table_insert(filenames, (gpointer)filename, NULL);
> +
> +    if (file_chain_loop_check(filenames, backing_file, backing_fmt)) {
> +        goto err;
> +    }
> +
> +    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..0945c09 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -378,6 +378,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_chain_okay(const char *filename, const char *fmt);
> +bool bdrv_new_chain_okay(const char *filename, const char *backing_file,
> +                            const char *backing_fmt);

Please align arguments:     ^

> +
>  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..d5ec45b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1641,11 +1641,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 +1658,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 +1700,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;
>  }

Is backing chain loop still checked in collect_image_info_list()?

Fam

Re: [Qemu-devel] [PATCH V6 2/5] qemu-img: Add infinite loop checking in bdrv_new_open()

[Fam Zheng]

On Tue, 11/05 22:09, Xu Wang wrote:
> Every image should be checked if there is infinite loop in backing
> file chain before open it. So infinite loop check was added into
> bdrv_new_open(). If @filename is opened without the flag
> BDRV_O_NO_BACKING, the infinite loop check should be called.
> 
> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>

I think this should be squashed into patch 1 so that the checking in
collect_image_info_list is kept along the series.

Fam

> ---
>  qemu-img.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index d5ec45b..3af7996 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)) {
> +        error_report("bdrv_new_open: Open %s failed. There is 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,
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH V6 3/5] block: Add check infinite loop in bdrv_img_create()

[Fam Zheng]

On Tue, 11/05 22:09, Xu Wang wrote:
> Backing file loop should be checked before qemu-img create command
> execution. If loop is found, qemu-img create should be stopped and
> an error printed.
> 
> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
> ---
>  block.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3443117..8423e80 100644
> --- a/block.c
> +++ b/block.c
> @@ -4670,15 +4670,15 @@ void bdrv_img_create(const char *filename, const char *fmt,
>      }
>  
>      backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> +    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
>      if (backing_file && backing_file->value.s) {
> -        if (!strcmp(filename, backing_file->value.s)) {
> -            error_setg(errp, "Error: Trying to create an image with the "
> -                             "same filename as the backing file");
> +        if (!bdrv_new_chain_okay(filename, backing_file->value.s,
> +                                backing_fmt->value.s)) {
> +            error_report("bdrv_img_create: loop exists, image create failed");

Please use error_setg(errp, "...");

Fam

Re: [Qemu-devel] [PATCH V6 5/5] blockdev: Add infinite loop check in drive_init()

[Fam Zheng]

On Tue, 11/05 22:09, Xu Wang wrote:
> Check the backing file for a loop during image boot, to avoid a lack or
> response or segfault.
> 
> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
> ---
>  blockdev.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index b260477..7c0927f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -510,6 +510,12 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
>  
>      bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>  
> +    /* Add backing file loop check */
> +    if (!bdrv_backing_chain_okay(file, drv ? drv->format_name : NULL)) {
> +        error_report("drive_init: backing file loop check failed");

Please use error_setg("...");

Fam

> +        goto err;
> +    }
> +
>      QINCREF(bs_opts);
>      ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
>  
> -- 
> 1.8.1.4
> 

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

[Eric Blake]

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

On 11/08/2013 03:19 AM, Fam Zheng wrote:
> 
>> +    BlockDriverState *bs;
>> +    BlockDriver *drv;
>> +    char fbuf[1024];
> 
> Could use PATH_MAX.

PATH_MAX is undefined on some platforms, and could also be defined to
something larger than a page which could lead to nastiness if you end up
overflowing the stack.  I personally prefer malloc'd buffers rather than
attempting to guess at how large to size things, although the rest of
the code base also has similar caps at 1024 so this isn't making it worse.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Jeff Cody]

On Fri, Nov 08, 2013 at 06:53:27AM -0700, Eric Blake wrote:
> On 11/08/2013 03:19 AM, Fam Zheng wrote:
> > 
> >> +    BlockDriverState *bs;
> >> +    BlockDriver *drv;
> >> +    char fbuf[1024];
> > 
> > Could use PATH_MAX.
> 
> PATH_MAX is undefined on some platforms, and could also be defined to
> something larger than a page which could lead to nastiness if you end up
> overflowing the stack.  I personally prefer malloc'd buffers rather than
> attempting to guess at how large to size things, although the rest of
> the code base also has similar caps at 1024 so this isn't making it worse.
>

A quick grep through the code shows ~57 arrays allocated using 1024,
and ~63 allocated using PATH_MAX.  Clearly not all of the 1024
allocation cases are pathname related, but certainly some of them are.

Maybe it makes sense to have a QEMU_PATH_MAX defined to 1024 in qemu,
so at least we are consistent everywhere.  (To clarify for Xu, I am
not talking about this patch series at all, just in general).

-Jeff

Re: [Qemu-devel] [PATCH V6 2/5] qemu-img: Add infinite loop checking in bdrv_new_open()

[Jeff Cody]

On Tue, Nov 05, 2013 at 10:09:18PM -0500, Xu Wang wrote:
> Every image should be checked if there is infinite loop in backing
> file chain before open it. So infinite loop check was added into
> bdrv_new_open(). If @filename is opened without the flag
> BDRV_O_NO_BACKING, the infinite loop check should be called.
> 
> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
> ---
>  qemu-img.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index d5ec45b..3af7996 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)) {
> +        error_report("bdrv_new_open: Open %s failed. There is loop exists "
> +                     "in the backing chain", filename);

I suggest rewording this for grammar; something like:
 +        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,
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH V6 2/5] qemu-img: Add infinite loop checking in bdrv_new_open()

[Stefan Weil]

See more suggestions below.

Am 08.11.2013 17:19, schrieb Jeff Cody:
> On Tue, Nov 05, 2013 at 10:09:18PM -0500, Xu Wang wrote:
>> Every image should be checked if there is infinite loop in backing
>> file chain before open it. So infinite loop check was added into.

... if there is an infinite loop in the backing
file chain before opening it. ...

>> bdrv_new_open(). If @filename is opened without the flag
>> BDRV_O_NO_BACKING, the infinite loop check should be called.
>>
>> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
>> ---
>>  qemu-img.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index d5ec45b..3af7996 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 the whole chain needs to be opened ...

>> +    if (!(flags & BDRV_O_NO_BACKING) &&
>> +        !bdrv_backing_chain_okay(filename, fmt)) {
>> +        error_report("bdrv_new_open: Open %s failed. There is loop exists "
>> +                     "in the backing chain", filename);
> I suggest rewording this for grammar; something like:
>  +        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,
>> -- 
>> 1.8.1.4