[Qemu-devel] [PATCH V9 0/4] Refine and export backing file loop check

[Qemu-devel] [PATCH V9 0/4] 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 V8:
  1. Delete duplicate info (such as @filename) in the error_setg().
  2. Change parameter from @fmt into @drv.

Updates from V7:
  1. Replace all error_report() in bdrv_backing_chain_okay() with error_setg().
  2. Fix error output in bdrv_img_create() when backing file doesn't exist.
  3. Add output in the bdrv_change_backing_file() (if not errp would has no
     way out).

Updates from V6:
  1. Combine bdrv_backing_chain_okay() and bdrv_new_chain_okay() into one.
  2. Delete bdrv_find_format() in the backing_chain_has_loop().
  3. Comments syntax and function naming updates.

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 (4):
  block/qemu-img: Refine and export infinite loop checking in
    collect_image_info_list()
  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               | 100 ++++++++++++++++++++++++++++++++++++++++++++++----
 blockdev.c            |   7 ++++
 include/block/block.h |   3 ++
 qemu-img.c            |  52 +++++++++++++-------------
 4 files changed, 128 insertions(+), 34 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH V9 1/4] 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               | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  3 +++
 qemu-img.c            | 52 ++++++++++++++++++------------------
 3 files changed, 102 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index 382ea71..7016ce8 100644
--- a/block.c
+++ b/block.c
@@ -4497,6 +4497,79 @@ 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,
+                                BlockDriver *drv, Error **errp)
+{
+    BlockDriverState *bs;
+    char fbuf[PATH_MAX];
+    Error *local_err = NULL;
+    int ret;
+
+    while (filename && (filename[0] != '\0')) {
+        if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
+            error_setg(errp, "Backing file '%s' creates an infinite loop.",
+                       filename);
+            return true;
+        }
+        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, drv, &local_err);
+        if (ret < 0) {
+            error_setg(errp, "Could not open '%s': %s", filename,
+                       error_get_pretty(local_err));
+            return true;
+        }
+
+        bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf));
+        filename = fbuf;
+        drv = 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.
+ * @drv: topmost image driver(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, BlockDriver *drv,
+                             const char *new_filename, Error **errp)
+{
+    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, drv, errp)) {
+        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..f5e84dc 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, BlockDriver *drv,
+                             const char *new_filename, Error **errp);
+
 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 b6b5644..1f38267 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, drv, NULL, &local_err)) {
+        error_report("bdrv_new_open: Open %s failed: %s", filename,
+                     error_get_pretty(local_err));
+        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;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH V9 2/4] 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 | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 7016ce8..b8cea1c 100644
--- a/block.c
+++ b/block.c
@@ -4633,14 +4633,6 @@ void bdrv_img_create(const char *filename, const char *fmt,
     }
 
     backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
-    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");
-            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);
@@ -4651,6 +4643,16 @@ void bdrv_img_create(const char *filename, const char *fmt,
         }
     }
 
+    if (backing_file && backing_file->value.s) {
+        if (!bdrv_backing_chain_okay(backing_file->value.s,
+                                     backing_drv, filename,
+                                     &local_err)) {
+            error_setg(errp, "Failed to create image: %s",
+                       error_get_pretty(local_err));
+            goto out;
+        }
+    }
+
     // The size for the image must always be specified, with one exception:
     // If we are using a backing file, we can obtain the size from there
     size = get_option_parameter(param, BLOCK_OPT_SIZE);
-- 
1.8.1.4

[Qemu-devel] [PATCH V9 3/4] 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block.c b/block.c
index b8cea1c..87f7018 100644
--- a/block.c
+++ b/block.c
@@ -2075,6 +2075,7 @@ static void coroutine_fn wait_for_overlapping_requests(BlockDriverState *bs,
  * Return values:
  * 0        - success
  * -EINVAL  - backing format specified, but no file
+ * -EIO     - generic I/O error (may happen for all errors)
  * -ENOSPC  - can't update the backing file because no space is left in the
  *            image file header
  * -ENOTSUP - format driver doesn't support changing the backing file
@@ -2083,6 +2084,7 @@ int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt)
 {
     BlockDriver *drv = bs->drv;
+    Error *local_err = NULL;
     int ret;
 
     /* Backing file format doesn't make sense without a backing file */
@@ -2090,6 +2092,13 @@ int bdrv_change_backing_file(BlockDriverState *bs,
         return -EINVAL;
     }
 
+    /* Check if loop exists in backing files chain after change */
+    if (!bdrv_backing_chain_okay(backing_file, NULL, bs->filename, 
+                                 &local_err)) {
+        error_report("Backing file check: %s", error_get_pretty(local_err));
+        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 V9 4/4] 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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 330aa4a..e39fc27 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -511,6 +511,13 @@ 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, NULL, &error)) {
+        error_setg(errp, "drive_init: backing file loop check failed. %s",
+                   error_get_pretty(error));
+        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 V9 1/4] block/qemu-img: Refine and export infinite loop checking in collect_image_info_list()

[Jeff Cody]

On Tue, Nov 26, 2013 at 01:38:30AM -0500, 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               | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |  3 +++
>  qemu-img.c            | 52 ++++++++++++++++++------------------
>  3 files changed, 102 insertions(+), 26 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 382ea71..7016ce8 100644
> --- a/block.c
> +++ b/block.c
> @@ -4497,6 +4497,79 @@ 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,
> +                                BlockDriver *drv, Error **errp)
> +{
> +    BlockDriverState *bs;
> +    char fbuf[PATH_MAX];
> +    Error *local_err = NULL;
> +    int ret;
> +
> +    while (filename && (filename[0] != '\0')) {
> +        if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
> +            error_setg(errp, "Backing file '%s' creates an infinite loop.",
> +                       filename);
> +            return true;
> +        }
> +        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, drv, &local_err);
> +        if (ret < 0) {
> +            error_setg(errp, "Could not open '%s': %s", filename,
> +                       error_get_pretty(local_err));
> +            return true;

This leaks *bs at this point.

> +        }
> +
> +        bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf));
> +        filename = fbuf;
> +        drv = 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.
> + * @drv: topmost image driver(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, BlockDriver *drv,
> +                             const char *new_filename, Error **errp)
> +{
> +    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, drv, errp)) {
> +        goto err;
> +    }
> +
> +exit:
> +    g_hash_table_destroy(filenames);
> +    return true;
> +
> +err:
> +    g_hash_table_destroy(filenames);
> +    return false;
> +}
> +

Minor nit, but it would be nice to have a single cleanup path.  E.g.,
something like this:

bool ret;

    ...

exit:
    g_hash_table_destroy(filenames);
    return ret;

Then just set ret as appropriate in each 'if' statement in the function, and
jump to 'exit'.


>  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..f5e84dc 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, BlockDriver *drv,
> +                             const char *new_filename, Error **errp);
> +
>  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 b6b5644..1f38267 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, drv, NULL, &local_err)) {
> +        error_report("bdrv_new_open: Open %s failed: %s", filename,
> +                     error_get_pretty(local_err));
> +        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));

Between here, and the next hunk of this patch, there is a
bdrv_unref(bs) in the original code.  This becomes important later:

> @@ -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);
> +            }

This function will now most likely blow up on qemu-img info --backing-chain,
if the image file has any backing files. 

Edit: just checked; it does indeed segfault:

   # ~/deploy/bin/qemu-img info --backing-chain /tmp/snap3.qcow2
   Segmentation fault (core dumped)


This patch does not have the context to show it, however as mentioned
above, the bdrv_unref(bs) is performed, yet bs is still used
afterwards in this function.  Once bdrv_unref() has been called by a
function, the BDS that was unreferenced can no longer be used.



>          }
>      }
> -    g_hash_table_destroy(filenames);
> +
>      return head;
>  
>  err:
>      qapi_free_ImageInfoList(head);
> -    g_hash_table_destroy(filenames);
>      return NULL;
>  }
>  
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH V9 3/4] block: Add backing file loop check in change_backing_file()

[Jeff Cody]

On Tue, Nov 26, 2013 at 01:38:32AM -0500, Xu Wang wrote:
> 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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/block.c b/block.c
> index b8cea1c..87f7018 100644
> --- a/block.c
> +++ b/block.c
> @@ -2075,6 +2075,7 @@ static void coroutine_fn wait_for_overlapping_requests(BlockDriverState *bs,
>   * Return values:
>   * 0        - success
>   * -EINVAL  - backing format specified, but no file
> + * -EIO     - generic I/O error (may happen for all errors)
>   * -ENOSPC  - can't update the backing file because no space is left in the
>   *            image file header
>   * -ENOTSUP - format driver doesn't support changing the backing file
> @@ -2083,6 +2084,7 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>      const char *backing_file, const char *backing_fmt)
>  {
>      BlockDriver *drv = bs->drv;
> +    Error *local_err = NULL;
>      int ret;
>  
>      /* Backing file format doesn't make sense without a backing file */
> @@ -2090,6 +2092,13 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>          return -EINVAL;
>      }
>  
> +    /* Check if loop exists in backing files chain after change */
> +    if (!bdrv_backing_chain_okay(backing_file, NULL, bs->filename, 
> +                                 &local_err)) {
> +        error_report("Backing file check: %s", error_get_pretty(local_err));

You need to free local_err before returning (i.e.
error_free(local_err)).



> +        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
> 
> 

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

[Jeff Cody]

On Tue, Nov 26, 2013 at 01:38:30AM -0500, 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               | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |  3 +++
>  qemu-img.c            | 52 ++++++++++++++++++------------------
>  3 files changed, 102 insertions(+), 26 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 382ea71..7016ce8 100644
> --- a/block.c
> +++ b/block.c
> @@ -4497,6 +4497,79 @@ 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,
> +                                BlockDriver *drv, Error **errp)
> +{
> +    BlockDriverState *bs;
> +    char fbuf[PATH_MAX];
> +    Error *local_err = NULL;
> +    int ret;
> +
> +    while (filename && (filename[0] != '\0')) {
> +        if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
> +            error_setg(errp, "Backing file '%s' creates an infinite loop.",
> +                       filename);
> +            return true;
> +        }
> +        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, drv, &local_err);
> +        if (ret < 0) {
> +            error_setg(errp, "Could not open '%s': %s", filename,
> +                       error_get_pretty(local_err));

This also leaks local_err here - you should call error_free(local_err).

> +            return true;
> +        }
> +
> +        bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf));
> +        filename = fbuf;
> +        drv = 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.
> + * @drv: topmost image driver(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, BlockDriver *drv,
> +                             const char *new_filename, Error **errp)
> +{
> +    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, drv, errp)) {
> +        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..f5e84dc 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, BlockDriver *drv,
> +                             const char *new_filename, Error **errp);
> +
>  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 b6b5644..1f38267 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, drv, NULL, &local_err)) {
> +        error_report("bdrv_new_open: Open %s failed: %s", filename,
> +                     error_get_pretty(local_err));
> +        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;
>  }
>  
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH V9 2/4] block: Add check infinite loop in bdrv_img_create()

[Jeff Cody]

On Tue, Nov 26, 2013 at 01:38:31AM -0500, 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 | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 7016ce8..b8cea1c 100644
> --- a/block.c
> +++ b/block.c
> @@ -4633,14 +4633,6 @@ void bdrv_img_create(const char *filename, const char *fmt,
>      }
>  
>      backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> -    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");
> -            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);
> @@ -4651,6 +4643,16 @@ void bdrv_img_create(const char *filename, const char *fmt,
>          }
>      }
>  
> +    if (backing_file && backing_file->value.s) {
> +        if (!bdrv_backing_chain_okay(backing_file->value.s,
> +                                     backing_drv, filename,
> +                                     &local_err)) {
> +            error_setg(errp, "Failed to create image: %s",
> +                       error_get_pretty(local_err));
> +            goto out;

This one is OK, because at out: there is an error_propagate(), which will
see that errp is already set, and just free local_err at that point.

> +        }
> +    }
> +
>      // The size for the image must always be specified, with one exception:
>      // If we are using a backing file, we can obtain the size from there
>      size = get_option_parameter(param, BLOCK_OPT_SIZE);
> -- 
> 1.8.1.4
> 
> 

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

[Jeff Cody]

On Tue, Nov 26, 2013 at 01:38:33AM -0500, 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 | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 330aa4a..e39fc27 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -511,6 +511,13 @@ 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, NULL, &error)) {
> +        error_setg(errp, "drive_init: backing file loop check failed. %s",
> +                   error_get_pretty(error));

Leaks error.

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