[Qemu-devel] [PATCH V8 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 <address@hidden>
---
 block.c               | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  3 +++
 qemu-img.c            | 52 ++++++++++++++++++------------------
 3 files changed, 103 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index 58efb5b..24202c0 100644
--- a/block.c
+++ b/block.c
@@ -4490,6 +4490,80 @@ 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, 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, NULL, &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;
+        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, 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, fmt, 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..6c58d50 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, 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 bf3fb4f..66d0d38 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, &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