[Qemu-devel] [PATCH V3 0/7] Redefine and export backing file loop check

[Qemu-devel] [PATCH V3 0/7] Redefine 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 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 (7):
  block/qemu-img: Refine and export infinite loop checking in
    collect_image_info_list()
  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: Add backing file loop check in qcow2_change_backing_file()
  block: Add backing file loop check in qed_change_backing_file()

 block.c               | 207 ++++++++++++++++++++++++++++++++++++++++++++++++--
 block/qcow2.c         |   6 ++
 block/qed.c           |   7 ++
 blockdev.c            |   6 ++
 include/block/block.h |   4 +
 qemu-img.c            |  29 +++----
 6 files changed, 234 insertions(+), 25 deletions(-)

-- 
1.8.1.4

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

[Xu Wang]

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               | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  4 +++
 qemu-img.c            | 29 +++++----------
 3 files changed, 110 insertions(+), 20 deletions(-)

diff --git a/block.c b/block.c
index 183fec8..c1d060d 100644
--- a/block.c
+++ b/block.c
@@ -4433,6 +4433,103 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
     bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
 }
 
+static gboolean str_equal_func(gconstpointer a, gconstpointer b)
+{
+    return strcmp(a, b) == 0;
+}
+
+/**
+ * Check backing file chain if there is a loop in it and build list of
+ * ImageInfo if needed.
+ *
+ * @filename: topmost image filename, absolute or relative path is OK.
+ * @fmt: topmost image format (may be NULL to autodetect)
+ * @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..19cac5e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1620,11 +1620,6 @@ static void dump_human_image_info_list(ImageInfoList *list)
     }
 }
 
-static gboolean str_equal_func(gconstpointer a, gconstpointer b)
-{
-    return strcmp(a, b) == 0;
-}
-
 /**
  * Open an image file chain and return an ImageInfoList
  *
@@ -1642,24 +1637,19 @@ static ImageInfoList *collect_image_info_list(const char *filename,
                                               bool chain)
 {
     ImageInfoList *head = NULL;
+    BlockDriverState *bs;
+    ImageInfoList *elem;
     ImageInfoList **last = &head;
-    GHashTable *filenames;
+    ImageInfo *info;
     Error *err = NULL;
 
-    filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
+    /* check if loop exists and build ImageInfoList */
+    if (chain &&
+        bdrv_backing_file_loop_check(filename, fmt, NULL, NULL)) {
+        goto err;
+    }
 
     while (filename) {
-        BlockDriverState *bs;
-        ImageInfo *info;
-        ImageInfoList *elem;
-
-        if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
-            error_report("Backing file '%s' creates an infinite loop.",
-                         filename);
-            goto err;
-        }
-        g_hash_table_insert(filenames, (gpointer)filename, NULL);
-
         bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING,
                            false, false);
         if (!bs) {
@@ -1692,12 +1682,11 @@ static ImageInfoList *collect_image_info_list(const char *filename,
             }
         }
     }
-    g_hash_table_destroy(filenames);
+
     return head;
 
 err:
     qapi_free_ImageInfoList(head);
-    g_hash_table_destroy(filenames);
     return NULL;
 }
 
-- 
1.8.1.4

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

[Fam Zheng]

On Mon, 07/15 05:42, Xu Wang wrote:
> 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               | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |  4 +++
>  qemu-img.c            | 29 +++++----------
>  3 files changed, 110 insertions(+), 20 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 183fec8..c1d060d 100644
> --- a/block.c
> +++ b/block.c
> @@ -4433,6 +4433,103 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
>      bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
>  }
>  
> +static gboolean str_equal_func(gconstpointer a, gconstpointer b)
> +{
> +    return strcmp(a, b) == 0;
> +}
> +
> +/**
> + * Check backing file chain if there is a loop in it and build list of
> + * ImageInfo if needed.

Need fix for this? Image info built in collect_image_info_list, not
here, right?

Fam

> + *
> + * @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) {

[Qemu-devel] [PATCH V3 2/7] block: Add WIN32 platform support for backing_file_loop_check()

[Xu Wang]

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 | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 101 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index c1d060d..a06bb08 100644
--- a/block.c
+++ b/block.c
@@ -51,6 +51,10 @@
 
 #define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
 
+#ifdef _WIN32
+#define MAX_PATH_LEN 8192
+#endif
+
 typedef enum {
     BDRV_REQ_COPY_ON_READ = 0x1,
     BDRV_REQ_ZERO_WRITE   = 0x2,
@@ -4433,6 +4437,96 @@ 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, offet;
+    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;
+    }
+    fread(&flag, 4, 1, fd);
+    if (flag != 0x4c) {
+        error_report("%s is not a lnk file.", lnk_file);
+        fclose(fd);
+        return -1;
+    }
+    fseek(fd, 0x14, SEEK_SET);
+    fread(&flag, 4, 1, fd);
+    fseek(fd, 0x4c, SEEK_SET);
+
+    if (flag & 0x01) {
+        fread(&sflag, 2, 1, fd);
+        fseek(fd, sflag, SEEK_CUR);
+    }
+
+    offset = ftell(fd);
+    fseek(fd, offset + 0x10, SEEK_SET);
+    fread(&flag, 4, 1, fd);
+    fseek(fd, flag + offset, SEEK_SET);
+
+    do {
+        fread(&uch, 1, 1, fd);
+        filepath[i++] = uch;
+    } while ((i <= MAX_PATH_LEN) && (uch != '\0'));
+
+    fclose(fd);
+    return 0;
+}
+#endif
+
+static long get_inode(const char *filename)
+{
+    #ifdef _WIN32
+    char pbuf[MAX_PATH_LEN], *p;
+    long inode;
+    struct stat sbuf;
+    char path[MAX_PATH_LEN];
+    int len;
+
+    /* If filename contains .lnk, it's a shortcuts. Target file
+     * need to be parsed.
+     */
+    len = strlen(filename);
+    if (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, MAX_PATH_LEN, 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;
@@ -4457,7 +4551,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];
@@ -4469,11 +4562,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;
@@ -4482,11 +4575,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

Re: [Qemu-devel] [PATCH V3 2/7] block: Add WIN32 platform support for backing_file_loop_check()

[Fam Zheng]

On Mon, 07/15 05:42, Xu Wang wrote:
> 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 | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 101 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c1d060d..a06bb08 100644
> --- a/block.c
> +++ b/block.c
> @@ -51,6 +51,10 @@
>  
>  #define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
>  
> +#ifdef _WIN32
> +#define MAX_PATH_LEN 8192
> +#endif
> +
>  typedef enum {
>      BDRV_REQ_COPY_ON_READ = 0x1,
>      BDRV_REQ_ZERO_WRITE   = 0x2,
> @@ -4433,6 +4437,96 @@ 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, offet;
> +    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;
> +    }
> +    fread(&flag, 4, 1, fd);
> +    if (flag != 0x4c) {
> +        error_report("%s is not a lnk file.", lnk_file);
> +        fclose(fd);
> +        return -1;
> +    }
> +    fseek(fd, 0x14, SEEK_SET);
> +    fread(&flag, 4, 1, fd);
> +    fseek(fd, 0x4c, SEEK_SET);
> +
> +    if (flag & 0x01) {
> +        fread(&sflag, 2, 1, fd);
> +        fseek(fd, sflag, SEEK_CUR);
> +    }
> +
> +    offset = ftell(fd);
> +    fseek(fd, offset + 0x10, SEEK_SET);
> +    fread(&flag, 4, 1, fd);
> +    fseek(fd, flag + offset, SEEK_SET);
> +
> +    do {
> +        fread(&uch, 1, 1, fd);
> +        filepath[i++] = uch;
> +    } while ((i <= MAX_PATH_LEN) && (uch != '\0'));
> +
> +    fclose(fd);
> +    return 0;
> +}

You didn't check ret value for fseek and fread, what if they are not
succeesful?

> +#endif
> +
> +static long get_inode(const char *filename)
> +{
> +    #ifdef _WIN32
> +    char pbuf[MAX_PATH_LEN], *p;
> +    long inode;
> +    struct stat sbuf;
> +    char path[MAX_PATH_LEN];
> +    int len;
> +
> +    /* If filename contains .lnk, it's a shortcuts. Target file
> +     * need to be parsed.
> +     */
> +    len = strlen(filename);
> +    if (strcmp(filename + len - 4, ".lnk")) {

Don't read out of range: if filename is shorter than 4 chars, (filename +
len - 4) points to address before filename.

> +        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, MAX_PATH_LEN, 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;
> @@ -4457,7 +4551,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];
> @@ -4469,11 +4562,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;
> @@ -4482,11 +4575,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
> 

-- 
Fam

[Qemu-devel] [PATCH V3 3/7] block: Check infinite loop in bdrv_img_create()

[Xu Wang]

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 a06bb08..e9b9112 100644
--- a/block.c
+++ b/block.c
@@ -4685,15 +4685,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)) {
+            /* There is loop exists in the backing file chain */
             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 V3 4/7] 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 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 e9b9112..bd8240c 100644
--- a/block.c
+++ b/block.c
@@ -1967,6 +1967,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 V3 5/7] block: Add infinite loop check in drive_init()

[Xu Wang]

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index b3a57e0..b5f063f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -695,6 +695,12 @@ 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)) {
+        goto err;
+    }
+
     ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv);
     bs_opts = NULL;
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH V3 5/7] block: Add infinite loop check in drive_init()

[Fam Zheng]

On Mon, 07/15 05:42, Xu Wang wrote:
> 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 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index b3a57e0..b5f063f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -695,6 +695,12 @@ 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)) {

Would you add an error_report here, as other error paths do?

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

-- 
Fam

[Qemu-devel] [PATCH V3 6/7] block: Add backing file loop check in qcow2_change_backing_file()

[Xu Wang]

Add backing file loop check before execute change backing file for
qcow2 format.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index 0eceefe..01bd7d5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1195,6 +1195,12 @@ fail:
 static int qcow2_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt)
 {
+    /* 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;
+    }
     pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_file ?: "");
     pstrcpy(bs->backing_format, sizeof(bs->backing_format), backing_fmt ?: "");
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH V3 6/7] block: Add backing file loop check in qcow2_change_backing_file()

[Fam Zheng]

On Mon, 07/15 05:42, Xu Wang wrote:
> Add backing file loop check before execute change backing file for
> qcow2 format.
> 
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  block/qcow2.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0eceefe..01bd7d5 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1195,6 +1195,12 @@ fail:
>  static int qcow2_change_backing_file(BlockDriverState *bs,
>      const char *backing_file, const char *backing_fmt)
>  {
> +    /* 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;
> +    }
>      pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_file ?: "");
>      pstrcpy(bs->backing_format, sizeof(bs->backing_format), backing_fmt ?: "");
>  
> -- 
> 1.8.1.4
> 

I'm not sure this is needed, there's already loop check in block layer's
bdrv_change_backing_file(), which calls drv->bdrv_change_backing_file().
wouldn't this be redundant?

Thanks.

-- 
Fam

[Qemu-devel] [PATCH V3 7/7] block: Add backing file loop check in qed_change_backing_file()

[Xu Wang]

Add backing file loop check before execute change backing file for
qed format.

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

diff --git a/block/qed.c b/block/qed.c
index f767b05..79c2875 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1468,6 +1468,13 @@ static int bdrv_qed_change_backing_file(BlockDriverState *bs,
         return -ENOTSUP;
     }
 
+    /* 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;
+    }
+
     memcpy(&new_header, &s->header, sizeof(new_header));
 
     new_header.features &= ~(QED_F_BACKING_FILE |
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH V3 0/7] Redefine and export backing file loop check

[Fam Zheng]

On Mon, 07/15 05:42, Xu Wang wrote:
> 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 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

Would you consider adding check in vmdk.c too, As it also supports
backing_hd?

Thanks.

Fam

> 
> Xu Wang (7):
>   block/qemu-img: Refine and export infinite loop checking in
>     collect_image_info_list()
>   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: Add backing file loop check in qcow2_change_backing_file()
>   block: Add backing file loop check in qed_change_backing_file()
> 
>  block.c               | 207 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  block/qcow2.c         |   6 ++
>  block/qed.c           |   7 ++
>  blockdev.c            |   6 ++
>  include/block/block.h |   4 +
>  qemu-img.c            |  29 +++----
>  6 files changed, 234 insertions(+), 25 deletions(-)
> 
> -- 
> 1.8.1.4
>