[Qemu-devel] [PATCH V5 0/6] Refine and export backing file loop check

[Qemu-devel] [PATCH V5 0/6] Refine and export backing file loop check

[Xu Wang]

From: Xu Wang <cngesaint@gmail.com>

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 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 (6):
  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 WIN32 platform support for backing_file_loop_check()
  block: Check infinite loop in bdrv_img_create()
  block: Add backing file loop check in change_backing_file()
  block: Add infinite loop check in drive_init()

 block.c               | 253 +++++++++++++++++++++++++++++++++++++++++++++++++-
 blockdev.c            |   7 ++
 include/block/block.h |   4 +
 qemu-img.c            |  51 +++++-----
 4 files changed, 284 insertions(+), 31 deletions(-)

-- 
1.8.1.4

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

[Xu Wang]

From: Xu Wang <cngesaint@gmail.com>

If there is a 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.
This patch 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().

Signed-off-by: Xu Wang <cngesaint@gmail.com>
---
 block.c               | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  4 +++
 qemu-img.c            | 44 ++++++++++-------------
 3 files changed, 118 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index 183fec8..aea653d 100644
--- a/block.c
+++ b/block.c
@@ -4433,6 +4433,102 @@ 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.
+ *
+ * @filename: topmost image filename, absolute or relative path is OK.
+ * @fmt: topmost image format (may be NULL to autodetect)
+ * @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
+ *
+ * Returns: true for backing file loop or error happened, false for no loop.
+ */
+bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
+                                  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) {
+                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);
+                bdrv_delete(bs);
+                goto err;
+            }
+        } else {
+            drv = NULL;
+        }
+
+        ret = bdrv_open(bs, filename, NULL,
+                        BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv);
+        if (ret < 0) {
+            error_report("Could not open '%s': %s", filename, strerror(-ret));
+            bdrv_delete(bs);
+            goto err;
+        }
+
+        bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf));
+        filename = fbuf;
+        fmt = NULL;
+
+        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 dd8eca1..3dc29bb 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -333,6 +333,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,
+                                  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 f8c97d3..4f01b0a 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,30 +1637,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));
@@ -1690,14 +1679,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 V5 2/6] qemu-img: Add infinite loop checking in bdrv_new_open()

[Xu Wang]

From: Xu Wang <cngesaint@gmail.com>

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 with no flags
BDRV_O_NO_BACKING, the infinite loop check should be called.

Signed-off-by: Xu Wang <cngesaint@gmail.com>
---
 qemu-img.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 4f01b0a..1a63bbf 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -278,6 +278,13 @@ 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_file_loop_check(filename, fmt, NULL, NULL)) {
+        error_report("bdrv_new_open: Open %s failed.", filename);
+        goto fail;
+    }
+
     ret = bdrv_open(bs, filename, NULL, flags, drv);
     if (ret < 0) {
         error_report("Could not open '%s': %s", filename, strerror(-ret));
-- 
1.8.1.4

[Qemu-devel] [PATCH V5 3/6] block: Add WIN32 platform support for backing_file_loop_check()

[Xu Wang]

From: Xu Wang <cngesaint@gmail.com>

Method of get_inode is different between Linux and WIN32 plateform.
This patch added inode caculate method on Windows plateform so that
backing file check could work on Windows plateform.

Signed-off-by: Xu Wang <cngesaint@gmail.com>
---
 block.c | 156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 148 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index aea653d..1e64665 100644
--- a/block.c
+++ b/block.c
@@ -4433,6 +4433,147 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
     bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
 }
 
+#ifdef _WIN32
+static int get_lnk_target_file(const char *lnk_file, char *filepath)
+{
+    unsigned int flag, offset;
+    unsigned int sflag;
+    char uch;
+    int i = 0;
+
+    FILE *fd = fopen(lnk_file, "rb");
+    if (!fd) {
+        error_report("Open file %s failed.", lnk_file);
+        return -1;
+    }
+
+    /* Check if the file is shortcuts by reading first 4 bytes if it's 0x4c */
+    if (fread(&flag, 4, 1, fd) != 1) {
+        error_report("Read 0x4c field of %s failed.", lnk_file);
+        goto err;
+    }
+
+    if (flag != 0x4c) {
+        error_report("%s is not a lnk file.", lnk_file);
+        goto err;
+    }
+
+    /* Parse flags on offset 0x14 */
+    if (fseek(fd, 0x14, SEEK_SET)) {
+        error_report("Seek flags of %s failed.", lnk_file);
+        goto err;
+    }
+
+    if (fread(&flag, 4, 1, fd) != 1) {
+        error_report("Read flags of %s failed.", lnk_file);
+        goto err;
+    }
+
+    /* seek to the length of item ID list */
+    if (fseek(fd, 0x4c, SEEK_SET)) {
+        error_report("Seek %s length of item ID list failed.", lnk_file);
+        goto err;
+    }
+
+    /* Parse Shell item ID list */
+    if (flag & 0x01) {
+        /* Get the length of SHITEMID */
+        if (fread(&sflag, 2, 1, fd) != 1) {
+            error_report("Read %s length of SHITEMID failed.", lnk_file);
+            goto err;
+        }
+
+        /* Skip SHITEMID */
+        if (fseek(fd, sflag, SEEK_CUR)) {
+            error_report("Skip SHITEMID of %s failed.", lnk_file);
+            goto err;
+        }
+    }
+
+    offset = ftell(fd);
+    if (offset == -1) {
+        error_report("ftell() of %s failed.", lnk_file);
+        goto err;
+    }
+    /* Read offset of local path */
+    if (fseek(fd, offset + 0x10, SEEK_SET)) {
+        error_report("Seek offset of %s failed.", lnk_file);
+        goto err;
+    }
+    if (fread(&flag, 4, 1, fd) != 1) {
+        error_report("Read offset of %s failed.", lnk_file);
+        goto err;
+    }
+    /* Seek to the start of target file path */
+    if (fseek(fd, flag + offset, SEEK_SET)) {
+        error_report("Seek target file path of %s failed.", lnk_file);
+        goto err;
+    }
+
+    do {
+        if (fread(&uch, 1, 1, fd) != 1) {
+            error_report("Read target path of %s failed.", lnk_file);
+            goto err;
+        }
+        filepath[i++] = uch;
+    } while ((i <= PATH_MAX) && (uch != '\0'));
+
+    fclose(fd);
+    return 0;
+
+err:
+    fclose(fd);
+    return -1;
+}
+#endif
+
+static long get_inode(const char *filename)
+{
+    #ifdef _WIN32
+    char pbuf[PATH_MAX + 1], *p;
+    long inode;
+    struct stat sbuf;
+    char path[PATH_MAX + 1];
+    int len;
+
+    /* If filename contains .lnk, it's a shortcuts. Target file
+     * need to be parsed.
+     */
+    len = strlen(filename);
+    if (len > 4 && !strcmp(filename + len - 4, ".lnk")) {
+        if (get_lnk_target_file(filename, path)) {
+            error_report("Parse .lnk file %s failed.", filename);
+            return -1;
+        }
+    } else {
+        memcpy(path, filename, sizeof(filename));
+    }
+
+    if (stat(path, &sbuf) == -1) {
+        error_report("get file %s stat error.", path);
+        return -1;
+    }
+    if (GetFullPathName(path, PATH_MAX + 1, pbuf, &p) != 0) {
+        inode = 11003;
+        for (p = pbuf; *p != '\0'; p++) {
+            inode = inode * 31 + *(unsigned char *)p;
+        }
+        return (inode * 911) & 0x7FFF;
+    }
+
+    return -1;
+    #else
+    struct stat sbuf;
+
+    if (stat(filename, &sbuf) == -1) {
+        error_report("get file %s stat error.", filename);
+        return -1;
+    }
+
+    return sbuf.st_ino;
+    #endif
+}
+
 static gboolean str_equal_func(gconstpointer a, gconstpointer b)
 {
     return strcmp(a, b) == 0;
@@ -4456,7 +4597,6 @@ bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
     GHashTable *inodes;
     BlockDriverState *bs;
     BlockDriver *drv;
-    struct stat sbuf;
     long inode = 0;
     int ret;
     char fbuf[1024];
@@ -4468,11 +4608,11 @@ bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
         if (access(filename, F_OK)) {
             inode = -1;
         } else {
-            if (stat(filename, &sbuf) == -1) {
-                error_report("Get file %s stat failed.", filename);
+            inode = get_inode(filename);
+            if (inode == -1) {
+                error_report("Get file %s inode failed.", filename);
                 goto err;
             }
-            inode = (long)sbuf.st_ino;
         }
 
         filename = backing_file;
@@ -4481,11 +4621,11 @@ bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
     }
 
     while (filename && (filename[0] != '\0')) {
-        if (stat(filename, &sbuf) == -1) {
-                error_report("Get file %s stat failed.", filename);
-                goto err;
+        inode = get_inode(filename);
+        if (inode == -1) {
+            error_report("Get file %s inode 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.",
-- 
1.8.1.4

[Qemu-devel] [PATCH V5 4/6] block: Check infinite loop in bdrv_img_create()

[Xu Wang]

From: Xu Wang <cngesaint@gmail.com>

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

Signed-off-by: Xu Wang <cngesaint@gmail.com>
---
 block.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 1e64665..8bb63c2 100644
--- a/block.c
+++ b/block.c
@@ -4731,15 +4731,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_backing_file_loop_check(filename, fmt,
+                                         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 V5 5/6] block: Add backing file loop check in change_backing_file()

[Xu Wang]

From: Xu Wang <cngesaint@gmail.com>

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

Signed-off-by: Xu Wang <cngesaint@gmail.com>
---
 block.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block.c b/block.c
index 8bb63c2..a14488f 100644
--- a/block.c
+++ b/block.c
@@ -1963,6 +1963,13 @@ int bdrv_change_backing_file(BlockDriverState *bs,
         return -EINVAL;
     }
 
+    /* Check if loop exists in backing files chain after changed */
+    if (bdrv_backing_file_loop_check(bs->filename,
+                                     bs->drv ? bs->drv->format_name : NULL,
+                                     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 V5 6/6] block: Add infinite loop check in drive_init()

[Xu Wang]

From: Xu Wang <cngesaint@gmail.com>

Backing file should be checked if there is a loop in it during image
boot. Becase if there is loop qemu would no response for a long time
and segment fault occured. So this patch would check backing file
chain if there is loop in it before open image.

Signed-off-by: Xu Wang <cngesaint@gmail.com>
---
 blockdev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index b3a57e0..590193f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -695,6 +695,13 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
         error_report("warning: disabling copy_on_read on readonly drive");
     }
 
+    /* Add backing file loop check */
+    if (bdrv_backing_file_loop_check(file, drv ? drv->format_name : NULL,
+                                     NULL, NULL)) {
+        error_report("drive_init: backing file loop check failed!");
+        goto err;
+    }
+
     ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv);
     bs_opts = NULL;
 
-- 
1.8.1.4

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

[Eric Blake]

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

On 08/02/2013 03:02 AM, Xu Wang wrote:
> From: Xu Wang <cngesaint@gmail.com>
> 
> If there is a 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.
> This patch 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().

Are we trying to get this series into 1.6 on the grounds that it fixes
bugs?  The grammar was a bit awkward; may I suggest:

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 an independent function
bdrv_backing_file_loop_check().

> 
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  block.c               | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |  4 +++
>  qemu-img.c            | 44 ++++++++++-------------
>  3 files changed, 118 insertions(+), 26 deletions(-)
> 

>  
> +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.
> + *
> + * @filename: topmost image filename, absolute or relative path is OK.
> + * @fmt: topmost image format (may be NULL to autodetect)
> + * @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.

This is confusing - are we passing in a filename or an inode?  At the
parameter level, it doesn't matter what we hash or even that we have a
hash table - just what we pass in.  The choice of hashing an inode value
is an internal detail.

> + * @backing_format: format of backing file

Why do we need to pass in two filenames?  I'm guessing it's because you
want to detect TWO types of loops:

Loop 1 - we are creating a NEW file, but requesting a backing file name
- if anything in that backing file chain points to the file we are about
to create, we would be creating a loop, and want to fail.  That is, if
we have an existing file "B" that points to a missing backing file "A",
and we are now creating file "A" with a backing file of "B", we want the
creation to fail because it would create a loop.  But in this case, it
is sufficient to note that "B" is a broken image (since "A" doesn't
exist yet), so we don't have to do a loop check here, rather we can just
merely test if "B" and its entire backing chain is reachable.

Loop 2 - we are testing if an EXISTING file has a loop.  But the loop
may not necessarily point back to our own file.  That is, a file "A"
with metadata that claims a backing file of "A" is a loop.  Also, a file
"A" with metadata that claims "B" as backing, and existing "B" with
metadata that claims "A" as backing, is a loop.  But we ALSO want to
detect and reject a file "A" that claims "B" as backing, where an
existing file "B" claims itself as backing; or even "A" claims "B", "B"
claims "C", and "C" claims "B".  Thus, we need to detect the loop no
matter where in the chain it occurs, and realize that it does not
necessarily point back to "A".  So, why not just declare that we are
starting with "A", without regards to it's backing file?

That is, I think your function signature is over-engineered - you really
only need to pass in a single file name - that of an existing file, and
have the file report success if all backing files can be resolved
without hitting any loops in the resolution, and failure if any backing
file is missing or if any backing file refers back to a point earlier in
the chain.

> + *
> + * Returns: true for backing file loop or error happened, false for no loop.
> + */
> +bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,

Those return semantics are a bit unusual.  A function named _check()
that returns a bool usually returns true when the function passed.  How
will this function usually be called?  Let's name it something that will
avoid double-negatives; maybe:

bool bdrv_backing_chain_okay(const char *filename, const char *fmt)

which returns true if filename is safe to use as a backing chain.

> +                                  const char *backing_file,
> +                                  const char *backing_format) {
> +    GHashTable *inodes;
> +    BlockDriverState *bs;
> +    BlockDriver *drv;
> +    struct stat sbuf;
> +    long inode = 0;

You are not guaranteed that st_ino fits in a long; use ino_t.  (IIRC, on
32-bit Cygwin, ino_t is 64-bit, but long is 32-bit)

> +    int ret;
> +    char fbuf[1024];
> +
> +    inodes = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);

So this says you are comparing by name (you are doing strcmp on the
hashed data)...

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

...but here, you are shoving in integers cast as a name.  strcmp() on
integers will not work.  Furthermore, inode alone is not guaranteed to
be unique; you are only guaranteed that the combination of dev_t and
ino_t form a unique identifier for any file (that is, you are prone to
false collisions if two files on different devices happen to have the
same inode).

I want to see loop detection working, but this patch still needs an
overhaul before it can be considered working.

-- 
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 V5 2/6] qemu-img: Add infinite loop checking in bdrv_new_open()

[Eric Blake]

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

On 08/02/2013 03:02 AM, Xu Wang wrote:
> From: Xu Wang <cngesaint@gmail.com>
> 
> 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 with no flags

s/with no flags/without the flag/

> BDRV_O_NO_BACKING, the infinite loop check should be called.
> 
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  qemu-img.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 4f01b0a..1a63bbf 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -278,6 +278,13 @@ 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_file_loop_check(filename, fmt, NULL, NULL)) {
> +        error_report("bdrv_new_open: Open %s failed.", filename);

Error messages generally don't end with '.'; also, this message seems to
lack details that might be useful to the end user (we know that either
the backing chain couldn't be completely followed or has a loop).

-- 
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 V5 3/6] block: Add WIN32 platform support for backing_file_loop_check()

[Eric Blake]

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

On 08/02/2013 03:02 AM, Xu Wang wrote:
> From: Xu Wang <cngesaint@gmail.com>
> 
> Method of get_inode is different between Linux and WIN32 plateform.

s/plateform/platform/g (3 instances in the commit message)

> This patch added inode caculate method on Windows plateform so that

s/added/adds an/
s/caculate/calculation/

> backing file check could work on Windows plateform.
> 
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  block.c | 156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 148 insertions(+), 8 deletions(-)
> 

>  }
>  
> +#ifdef _WIN32
> +static int get_lnk_target_file(const char *lnk_file, char *filepath)
> +{
> +    unsigned int flag, offset;
> +    unsigned int sflag;
> +    char uch;
> +    int i = 0;
> +
> +    FILE *fd = fopen(lnk_file, "rb");
> +    if (!fd) {
> +        error_report("Open file %s failed.", lnk_file);

Error messages should not include '.'; also, when it is due to a system
call failure, you should include strerror() information explaining the
failure.

> +        return -1;
> +    }
> +
> +    /* Check if the file is shortcuts by reading first 4 bytes if it's 0x4c */
> +    if (fread(&flag, 4, 1, fd) != 1) {
> +        error_report("Read 0x4c field of %s failed.", lnk_file);

I don't know WIN32 programming well enough to know if this really how
you should be checking for infinite loops.  But how is this supposed to
work?  In POSIX, fopen() of a symlink opens its destination; to read a
symlink's contents, you have to use readlink() (that is, the API used to
read file contents is NOT the API used to chase link resolution).  This
whole function feels like a horrible hack, unlikely to do what you meant.


> +
> +static long get_inode(const char *filename)

Again, 'long' and 'ino_t' are not necessarily compatible types.  Use
ino_t.  And again, 'ino_t' alone does not uniquely designate a file; it
is the combination of device and inode numbers together that give
uniqueness.

> +{
> +    #ifdef _WIN32

We usually anchor # in the first column.

> +    char pbuf[PATH_MAX + 1], *p;
> +    long inode;
> +    struct stat sbuf;
> +    char path[PATH_MAX + 1];
> +    int len;
> +
> +    /* If filename contains .lnk, it's a shortcuts. Target file
> +     * need to be parsed.

How does the rest of the qemu code base handle .lnk files?  Are we
trying to dereference the target file automatically?  If so, is there
already code that does that, which we can reuse here?  It just seems
awkward that you are implementing this from scratch - either we support
reading through windows links (and should reuse that code) or we don't
(and hence it doesn't matter if the user passes us a .lnk file, we
aren't treating it any different than any other data file).

-- 
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 V5 4/6] block: Check infinite loop in bdrv_img_create()

[Eric Blake]

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

On 08/02/2013 03:02 AM, Xu Wang wrote:
> From: Xu Wang <cngesaint@gmail.com>
> 
> Backing file loop should be checked before qemu-img create command
> execution. If loop was found, qemu-img create should be stopped and

s/was/is/ - commit messages should generally be in present tense (what
the code is doing now that the patch is applied) not in past tense.

> an error was printed.

s/was //

> 
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  block.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 

>  
>      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_backing_file_loop_check(filename, fmt,
> +                                         backing_file->value.s,
> +                                         backing_fmt->value.s)) {
> +            error_report("bdrv_img_create: loop exists, image create failed.");

No '.' in error messages.

-- 
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 V5 5/6] block: Add backing file loop check in change_backing_file()

[Eric Blake]

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

On 08/02/2013 03:02 AM, Xu Wang wrote:
> From: Xu Wang <cngesaint@gmail.com>
> 
> Backing file loop should be checked before calling change_backing_
> file(). If loop appeared, this calling should be stopped and an

Breaking a function name across a line break is awkward.

> error was printed.

s/was //

> 
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  block.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

>  
> +    /* Check if loop exists in backing files chain after changed */

s/changed/change/

But isn't that a bit late?  Don't you want to check that a backing loop
will not be created, prior to making the change?

> +    if (bdrv_backing_file_loop_check(bs->filename,
> +                                     bs->drv ? bs->drv->format_name : NULL,
> +                                     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 {
> 

-- 
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 V5 6/6] block: Add infinite loop check in drive_init()

[Eric Blake]

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

On 08/02/2013 03:02 AM, Xu Wang wrote:
> From: Xu Wang <cngesaint@gmail.com>
> 
> Backing file should be checked if there is a loop in it during image
> boot. Becase if there is loop qemu would no response for a long time

s/Becase/Because/

> and segment fault occured. So this patch would check backing file

s/occured/occurred/

> chain if there is loop in it before open image.

I'd suggest:

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

> 
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  blockdev.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index b3a57e0..590193f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -695,6 +695,13 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>          error_report("warning: disabling copy_on_read on readonly drive");
>      }
>  
> +    /* Add backing file loop check */
> +    if (bdrv_backing_file_loop_check(file, drv ? drv->format_name : NULL,
> +                                     NULL, NULL)) {
> +        error_report("drive_init: backing file loop check failed!");

The '!' seems a bit of overkill; it is sufficient to give an error
message without trailing punctuation.

> +        goto err;
> +    }
> +
>      ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv);
>      bs_opts = NULL;
>  
> 

-- 
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 V5 1/6] block/qemu-img: Refine and export infinite loop checking in collect_image_info_list()

[Xu Wang]

于 2013/8/3 6:09, Eric Blake 写道:
> On 08/02/2013 03:02 AM, Xu Wang wrote:
>> From: Xu Wang<cngesaint@gmail.com>
>>
>> If there is a 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.
>> This patch 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().
> Are we trying to get this series into 1.6 on the grounds that it fixes
> bugs?  The grammar was a bit awkward; may I suggest:
>
> 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 an independent function
> bdrv_backing_file_loop_check().
I am very sorry for working other works so long time and do not submit new
version patches in time. Now I am rewriting them to improve their quality.
>> Signed-off-by: Xu Wang<cngesaint@gmail.com>
>> ---
>>   block.c               | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/block/block.h |  4 +++
>>   qemu-img.c            | 44 ++++++++++-------------
>>   3 files changed, 118 insertions(+), 26 deletions(-)
>>
>>   
>> +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.
>> + *
>> + * @filename: topmost image filename, absolute or relative path is OK.
>> + * @fmt: topmost image format (may be NULL to autodetect)
>> + * @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.
> This is confusing - are we passing in a filename or an inode?  At the
> parameter level, it doesn't matter what we hash or even that we have a
> hash table - just what we pass in.  The choice of hashing an inode value
> is an internal detail.
In the new version I'll just pass filename and compare them to find loop.
>> + * @backing_format: format of backing file
> Why do we need to pass in two filenames?  I'm guessing it's because you
> want to detect TWO types of loops:
>
> Loop 1 - we are creating a NEW file, but requesting a backing file name
> - if anything in that backing file chain points to the file we are about
> to create, we would be creating a loop, and want to fail.  That is, if
> we have an existing file "B" that points to a missing backing file "A",
> and we are now creating file "A" with a backing file of "B", we want the
> creation to fail because it would create a loop.  But in this case, it
> is sufficient to note that "B" is a broken image (since "A" doesn't
> exist yet), so we don't have to do a loop check here, rather we can just
> merely test if "B" and its entire backing chain is reachable.
>
> Loop 2 - we are testing if an EXISTING file has a loop.  But the loop
> may not necessarily point back to our own file.  That is, a file "A"
> with metadata that claims a backing file of "A" is a loop.  Also, a file
> "A" with metadata that claims "B" as backing, and existing "B" with
> metadata that claims "A" as backing, is a loop.  But we ALSO want to
> detect and reject a file "A" that claims "B" as backing, where an
> existing file "B" claims itself as backing; or even "A" claims "B", "B"
> claims "C", and "C" claims "B".  Thus, we need to detect the loop no
> matter where in the chain it occurs, and realize that it does not
> necessarily point back to "A".  So, why not just declare that we are
> starting with "A", without regards to it's backing file?
>
> That is, I think your function signature is over-engineered - you really
> only need to pass in a single file name - that of an existing file, and
> have the file report success if all backing files can be resolved
> without hitting any loops in the resolution, and failure if any backing
> file is missing or if any backing file refers back to a point earlier in
> the chain.
If we execute 'qemu-img create ...' command to create a new image and point
set a backing file. I should check if there is loop exists in the 
backing file
chain (as you said above). But this function also need to verify the new
image to be created won't overwrite any one in the backing file chain. 
Because
that will create a loop. If I check the whole chain after the new image 
created,
it's too late to find the loop (if it exists) because some image has 
been overwrited.
So is there any suggestion if I just pass one filename to the function? 
Thanks.
>> + *
>> + * Returns: true for backing file loop or error happened, false for no loop.
>> + */
>> +bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
> Those return semantics are a bit unusual.  A function named _check()
> that returns a bool usually returns true when the function passed.  How
> will this function usually be called?  Let's name it something that will
> avoid double-negatives; maybe:
>
> bool bdrv_backing_chain_okay(const char *filename, const char *fmt)
>
> which returns true if filename is safe to use as a backing chain.
Good name. I'll change name of this function into it:-)
>
>> +                                  const char *backing_file,
>> +                                  const char *backing_format) {
>> +    GHashTable *inodes;
>> +    BlockDriverState *bs;
>> +    BlockDriver *drv;
>> +    struct stat sbuf;
>> +    long inode = 0;
> You are not guaranteed that st_ino fits in a long; use ino_t.  (IIRC, on
> 32-bit Cygwin, ino_t is 64-bit, but long is 32-bit)
Sure. I'll update all points like this.
>> +    int ret;
>> +    char fbuf[1024];
>> +
>> +    inodes = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
> So this says you are comparing by name (you are doing strcmp on the
> hashed data)...
Comparing by name is good choice. I'll update it in my new version.
>> +
>> +    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);
> ...but here, you are shoving in integers cast as a name.  strcmp() on
> integers will not work.  Furthermore, inode alone is not guaranteed to
> be unique; you are only guaranteed that the combination of dev_t and
> ino_t form a unique identifier for any file (that is, you are prone to
> false collisions if two files on different devices happen to have the
> same inode).
> I want to see loop detection working, but this patch still needs an
> overhaul before it can be considered working.
Thank you very much for your suggestions:-)
   Xu Wang