[Qemu-devel] [PATCH 0/5] Add qemu-img check subcommand

[Qemu-devel] [PATCH 0/5] Add qemu-img check subcommand

[Kevin Wolf]

Current qcow2 code already includes some functions to do some consistency
checks (e.g. compare refcounts to real usage). Allowing the user (or developer
debugging qcow2 code) to check his images only involves actually building this
code and exposing it to the user by qemu-img.

This patch series implements a check subcommand for qemu-img. I think it will
be useful especially in combination with qemu-io for test suites.

Kevin

Kevin Wolf (5):
  qcow2: Fix warnings in check_refcount()
  Introduce bdrv_check
  Introduce qemu-img check subcommand
  qcow2: Refcount checking code cleanup
  qcow2: Add plausibility check for L1/L2 entries

 block-qcow2.c |  239 +++++++++++++++++++++++++++++++++++++++++---------------
 block.c       |   14 ++++
 block.h       |    1 +
 block_int.h   |    3 +
 qemu-img.c    |   62 +++++++++++++++
 5 files changed, 255 insertions(+), 64 deletions(-)

[Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Kevin Wolf]

This code is currently only compiled when DEBUG_ALLOC is defined, so you 
usually don't see compiler warnings on it. This patch series wants to 
enable the code, so fix the format string warnings first.

While we're at it, let's print error messages to stderr.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[kwolf]

From: Kevin Wolf <kwolf@redhat.com>

This code is currently only compiled when DEBUG_ALLOC is defined, so you
usually don't see compiler warnings on it. This patch series wants to enable
the code, so fix the format string warnings first.

While we're at it, let's print error messages to stderr.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block-qcow2.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/block-qcow2.c b/block-qcow2.c
index 985214f..231b12f 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -2583,10 +2583,12 @@ static void inc_refcounts(BlockDriverState *bs,
         cluster_offset += s->cluster_size) {
         k = cluster_offset >> s->cluster_bits;
         if (k < 0 || k >= refcount_table_size) {
-            printf("ERROR: invalid cluster offset=0x%llx\n", cluster_offset);
+            fprintf(stderr, "ERROR: invalid cluster offset=0x%" PRIx64 "\n",
+                cluster_offset);
         } else {
             if (++refcount_table[k] == 0) {
-                printf("ERROR: overflow cluster offset=0x%llx\n", cluster_offset);
+                fprintf(stderr, "ERROR: overflow cluster offset=0x%" PRIx64
+                    "\n", cluster_offset);
             }
         }
     }
@@ -2623,8 +2625,8 @@ static int check_refcounts_l1(BlockDriverState *bs,
             if (check_copied) {
                 refcount = get_refcount(bs, (l2_offset & ~QCOW_OFLAG_COPIED) >> s->cluster_bits);
                 if ((refcount == 1) != ((l2_offset & QCOW_OFLAG_COPIED) != 0)) {
-                    printf("ERROR OFLAG_COPIED: l2_offset=%llx refcount=%d\n",
-                           l2_offset, refcount);
+                    fprintf(stderr, "ERROR OFLAG_COPIED: l2_offset=%" PRIx64
+                        " refcount=%d\n", l2_offset, refcount);
                 }
             }
             l2_offset &= ~QCOW_OFLAG_COPIED;
@@ -2635,8 +2637,9 @@ static int check_refcounts_l1(BlockDriverState *bs,
                 if (offset != 0) {
                     if (offset & QCOW_OFLAG_COMPRESSED) {
                         if (offset & QCOW_OFLAG_COPIED) {
-                            printf("ERROR: cluster %lld: copied flag must never be set for compressed clusters\n",
-                                   offset >> s->cluster_bits);
+                            fprintf(stderr, "ERROR: cluster %" PRId64 ": "
+                                "copied flag must never be set for compressed "
+                                "clusters\n", offset >> s->cluster_bits);
                             offset &= ~QCOW_OFLAG_COPIED;
                         }
                         nb_csectors = ((offset >> s->csize_shift) &
@@ -2649,8 +2652,8 @@ static int check_refcounts_l1(BlockDriverState *bs,
                         if (check_copied) {
                             refcount = get_refcount(bs, (offset & ~QCOW_OFLAG_COPIED) >> s->cluster_bits);
                             if ((refcount == 1) != ((offset & QCOW_OFLAG_COPIED) != 0)) {
-                                printf("ERROR OFLAG_COPIED: offset=%llx refcount=%d\n",
-                                       offset, refcount);
+                                fprintf(stderr, "ERROR OFLAG_COPIED: offset=%"
+                                    PRIx64 " refcount=%d\n", offset, refcount);
                             }
                         }
                         offset &= ~QCOW_OFLAG_COPIED;
@@ -2670,7 +2673,7 @@ static int check_refcounts_l1(BlockDriverState *bs,
     qemu_free(l2_table);
     return 0;
  fail:
-    printf("ERROR: I/O error in check_refcounts_l1\n");
+    fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n");
     qemu_free(l1_table);
     qemu_free(l2_table);
     return -EIO;
@@ -2722,7 +2725,7 @@ static void check_refcounts(BlockDriverState *bs)
         refcount1 = get_refcount(bs, i);
         refcount2 = refcount_table[i];
         if (refcount1 != refcount2)
-            printf("ERROR cluster %d refcount=%d reference=%d\n",
+            fprintf(stderr, "ERROR cluster %d refcount=%d reference=%d\n",
                    i, refcount1, refcount2);
     }
 
-- 
1.6.0.6

[Qemu-devel] [PATCH 2/5] Introduce bdrv_check

[kwolf]

From: Kevin Wolf <kwolf@redhat.com>

Introduce a new bdrv_check function pointer for block drivers. Modify qcow2 to
return an error status in check_refcounts(), so it can implement bdrv_check.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block-qcow2.c |   71 +++++++++++++++++++++++++++++++++++++++++---------------
 block.c       |   14 +++++++++++
 block.h       |    1 +
 block_int.h   |    3 ++
 4 files changed, 70 insertions(+), 19 deletions(-)

diff --git a/block-qcow2.c b/block-qcow2.c
index 231b12f..cbadcd0 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -177,9 +177,7 @@ static int64_t alloc_clusters(BlockDriverState *bs, int64_t size);
 static int64_t alloc_bytes(BlockDriverState *bs, int size);
 static void free_clusters(BlockDriverState *bs,
                           int64_t offset, int64_t size);
-#ifdef DEBUG_ALLOC
-static void check_refcounts(BlockDriverState *bs);
-#endif
+static int check_refcounts(BlockDriverState *bs);
 
 static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
@@ -2564,8 +2562,14 @@ static void update_refcount(BlockDriverState *bs,
     }
 }
 
-#ifdef DEBUG_ALLOC
-static void inc_refcounts(BlockDriverState *bs,
+/*
+ * Increases the refcount for a range of clusters in a given refcount table.
+ * This is used to construct a temporary refcount table out of L1 and L2 tables
+ * which can be compared the the refcount table saved in the image.
+ *
+ * Returns the number of errors in the image that were found
+ */
+static int inc_refcounts(BlockDriverState *bs,
                           uint16_t *refcount_table,
                           int refcount_table_size,
                           int64_t offset, int64_t size)
@@ -2573,9 +2577,10 @@ static void inc_refcounts(BlockDriverState *bs,
     BDRVQcowState *s = bs->opaque;
     int64_t start, last, cluster_offset;
     int k;
+    int errors = 0;
 
     if (size <= 0)
-        return;
+        return 0;
 
     start = offset & ~(s->cluster_size - 1);
     last = (offset + size - 1) & ~(s->cluster_size - 1);
@@ -2585,13 +2590,17 @@ static void inc_refcounts(BlockDriverState *bs,
         if (k < 0 || k >= refcount_table_size) {
             fprintf(stderr, "ERROR: invalid cluster offset=0x%" PRIx64 "\n",
                 cluster_offset);
+            errors++;
         } else {
             if (++refcount_table[k] == 0) {
                 fprintf(stderr, "ERROR: overflow cluster offset=0x%" PRIx64
                     "\n", cluster_offset);
+                errors++;
             }
         }
     }
+
+    return errors;
 }
 
 static int check_refcounts_l1(BlockDriverState *bs,
@@ -2603,11 +2612,12 @@ static int check_refcounts_l1(BlockDriverState *bs,
     BDRVQcowState *s = bs->opaque;
     uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2;
     int l2_size, i, j, nb_csectors, refcount;
+    int errors = 0;
 
     l2_table = NULL;
     l1_size2 = l1_size * sizeof(uint64_t);
 
-    inc_refcounts(bs, refcount_table, refcount_table_size,
+    errors += inc_refcounts(bs, refcount_table, refcount_table_size,
                   l1_table_offset, l1_size2);
 
     l1_table = qemu_malloc(l1_size2);
@@ -2627,6 +2637,7 @@ static int check_refcounts_l1(BlockDriverState *bs,
                 if ((refcount == 1) != ((l2_offset & QCOW_OFLAG_COPIED) != 0)) {
                     fprintf(stderr, "ERROR OFLAG_COPIED: l2_offset=%" PRIx64
                         " refcount=%d\n", l2_offset, refcount);
+                    errors++;
                 }
             }
             l2_offset &= ~QCOW_OFLAG_COPIED;
@@ -2641,11 +2652,12 @@ static int check_refcounts_l1(BlockDriverState *bs,
                                 "copied flag must never be set for compressed "
                                 "clusters\n", offset >> s->cluster_bits);
                             offset &= ~QCOW_OFLAG_COPIED;
+                            errors++;
                         }
                         nb_csectors = ((offset >> s->csize_shift) &
                                        s->csize_mask) + 1;
                         offset &= s->cluster_offset_mask;
-                        inc_refcounts(bs, refcount_table,
+                        errors += inc_refcounts(bs, refcount_table,
                                       refcount_table_size,
                                       offset & ~511, nb_csectors * 512);
                     } else {
@@ -2654,16 +2666,17 @@ static int check_refcounts_l1(BlockDriverState *bs,
                             if ((refcount == 1) != ((offset & QCOW_OFLAG_COPIED) != 0)) {
                                 fprintf(stderr, "ERROR OFLAG_COPIED: offset=%"
                                     PRIx64 " refcount=%d\n", offset, refcount);
+                                errors++;
                             }
                         }
                         offset &= ~QCOW_OFLAG_COPIED;
-                        inc_refcounts(bs, refcount_table,
+                        errors += inc_refcounts(bs, refcount_table,
                                       refcount_table_size,
                                       offset, s->cluster_size);
                     }
                 }
             }
-            inc_refcounts(bs, refcount_table,
+            errors += inc_refcounts(bs, refcount_table,
                           refcount_table_size,
                           l2_offset,
                           s->cluster_size);
@@ -2671,7 +2684,7 @@ static int check_refcounts_l1(BlockDriverState *bs,
     }
     qemu_free(l1_table);
     qemu_free(l2_table);
-    return 0;
+    return errors;
  fail:
     fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n");
     qemu_free(l1_table);
@@ -2679,24 +2692,35 @@ static int check_refcounts_l1(BlockDriverState *bs,
     return -EIO;
 }
 
-static void check_refcounts(BlockDriverState *bs)
+/*
+ * Checks an image for refcount consistency.
+ *
+ * Returns 0 if no errors are found, the number of errors in case the image is
+ * detected as corrupted, and -errno when an internal error occured.
+ */
+static int check_refcounts(BlockDriverState *bs)
 {
     BDRVQcowState *s = bs->opaque;
     int64_t size;
     int nb_clusters, refcount1, refcount2, i;
     QCowSnapshot *sn;
     uint16_t *refcount_table;
+    int ret, errors = 0;
 
     size = bdrv_getlength(s->hd);
     nb_clusters = size_to_clusters(s, size);
     refcount_table = qemu_mallocz(nb_clusters * sizeof(uint16_t));
 
     /* header */
-    inc_refcounts(bs, refcount_table, nb_clusters,
+    errors += inc_refcounts(bs, refcount_table, nb_clusters,
                   0, s->cluster_size);
 
-    check_refcounts_l1(bs, refcount_table, nb_clusters,
+    ret = check_refcounts_l1(bs, refcount_table, nb_clusters,
                        s->l1_table_offset, s->l1_size, 1);
+    if (ret < 0) {
+        return ret;
+    }
+    errors += ret;
 
     /* snapshots */
     for(i = 0; i < s->nb_snapshots; i++) {
@@ -2704,18 +2728,18 @@ static void check_refcounts(BlockDriverState *bs)
         check_refcounts_l1(bs, refcount_table, nb_clusters,
                            sn->l1_table_offset, sn->l1_size, 0);
     }
-    inc_refcounts(bs, refcount_table, nb_clusters,
+    errors += inc_refcounts(bs, refcount_table, nb_clusters,
                   s->snapshots_offset, s->snapshots_size);
 
     /* refcount data */
-    inc_refcounts(bs, refcount_table, nb_clusters,
+    errors += inc_refcounts(bs, refcount_table, nb_clusters,
                   s->refcount_table_offset,
                   s->refcount_table_size * sizeof(uint64_t));
     for(i = 0; i < s->refcount_table_size; i++) {
         int64_t offset;
         offset = s->refcount_table[i];
         if (offset != 0) {
-            inc_refcounts(bs, refcount_table, nb_clusters,
+            errors += inc_refcounts(bs, refcount_table, nb_clusters,
                           offset, s->cluster_size);
         }
     }
@@ -2724,12 +2748,21 @@ static void check_refcounts(BlockDriverState *bs)
     for(i = 0; i < nb_clusters; i++) {
         refcount1 = get_refcount(bs, i);
         refcount2 = refcount_table[i];
-        if (refcount1 != refcount2)
+        if (refcount1 != refcount2) {
             fprintf(stderr, "ERROR cluster %d refcount=%d reference=%d\n",
                    i, refcount1, refcount2);
+            errors++;
+        }
     }
 
     qemu_free(refcount_table);
+
+    return errors;
+}
+
+static int qcow_check(BlockDriverState *bs)
+{
+    return check_refcounts(bs);
 }
 
 #if 0
@@ -2751,7 +2784,6 @@ static void dump_refcounts(BlockDriverState *bs)
     }
 }
 #endif
-#endif
 
 static int qcow_put_buffer(BlockDriverState *bs, const uint8_t *buf,
                            int64_t pos, int size)
@@ -2806,4 +2838,5 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_get_buffer    = qcow_get_buffer,
 
     .bdrv_create2 = qcow_create2,
+    .bdrv_check = qcow_check,
 };
diff --git a/block.c b/block.c
index 836a6e5..8348cf2 100644
--- a/block.c
+++ b/block.c
@@ -506,6 +506,20 @@ void bdrv_delete(BlockDriverState *bs)
     qemu_free(bs);
 }
 
+/*
+ * Run consistency checks on an image
+ *
+ * Returns the number of errors or -errno when an internal error occurs
+ */
+int bdrv_check(BlockDriverState *bs)
+{
+    if (bs->drv->bdrv_check == NULL) {
+        return -ENOTSUP;
+    }
+
+    return bs->drv->bdrv_check(bs);
+}
+
 /* commit COW file into the raw image */
 int bdrv_commit(BlockDriverState *bs)
 {
diff --git a/block.h b/block.h
index ca672a1..5aef076 100644
--- a/block.h
+++ b/block.h
@@ -73,6 +73,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags);
 int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
                BlockDriver *drv);
 void bdrv_close(BlockDriverState *bs);
+int bdrv_check(BlockDriverState *bs);
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
               uint8_t *buf, int nb_sectors);
 int bdrv_write(BlockDriverState *bs, int64_t sector_num,
diff --git a/block_int.h b/block_int.h
index 3e78997..e10b906 100644
--- a/block_int.h
+++ b/block_int.h
@@ -102,6 +102,9 @@ struct BlockDriver {
                         const char *backing_file, const char *backing_format,
                         int flags);
 
+    /* Returns number of errors in image, -errno for internal errors */
+    int (*bdrv_check)(BlockDriverState* bs);
+
     struct BlockDriver *next;
 };
 
-- 
1.6.0.6

[Qemu-devel] [PATCH 3/5] Introduce qemu-img check subcommand

[kwolf]

From: Kevin Wolf <kwolf@redhat.com>

Now that block drivers can provide check functions, expose them through
qemu-img.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index ccf4a6f..29149a2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -58,6 +58,7 @@ static void help(void)
            "QEMU disk image utility\n"
            "\n"
            "Command syntax:\n"
+           "  check [-f fmt] filename\n"
            "  create [-e] [-6] [-F fmt] [-b base_image] [-f fmt] filename [size]\n"
            "  commit [-f fmt] filename\n"
            "  convert [-c] [-e] [-6] [-f fmt] [-O output_fmt] [-B output_base_image] filename [filename2 [...]] output_filename\n"
@@ -315,6 +316,65 @@ static int img_create(int argc, char **argv)
     return 0;
 }
 
+static int img_check(int argc, char **argv)
+{
+    int c, ret;
+    const char *filename, *fmt;
+    BlockDriver *drv;
+    BlockDriverState *bs;
+
+    fmt = NULL;
+    for(;;) {
+        c = getopt(argc, argv, "f:h");
+        if (c == -1)
+            break;
+        switch(c) {
+        case 'h':
+            help();
+            break;
+        case 'f':
+            fmt = optarg;
+            break;
+        }
+    }
+    if (optind >= argc)
+        help();
+    filename = argv[optind++];
+
+    bs = bdrv_new("");
+    if (!bs)
+        error("Not enough memory");
+    if (fmt) {
+        drv = bdrv_find_format(fmt);
+        if (!drv)
+            error("Unknown file format '%s'", fmt);
+    } else {
+        drv = NULL;
+    }
+    if (bdrv_open2(bs, filename, BRDV_O_FLAGS, drv) < 0) {
+        error("Could not open '%s'", filename);
+    }
+    ret = bdrv_check(bs);
+    switch(ret) {
+    case 0:
+        printf("No errors were found on the image.\n");
+        break;
+    case -ENOTSUP:
+        error("This image format does not support checks");
+        break;
+    default:
+        if (ret < 0) {
+            error("An error occurred during the check");
+        } else {
+            printf("%d errors were found on the image.\n", ret);
+        }
+        break;
+    }
+
+    bdrv_delete(bs);
+    return 0;
+}
+
 static int img_commit(int argc, char **argv)
 {
     int c, ret;
@@ -888,6 +948,8 @@ int main(int argc, char **argv)
     argc--; argv++;
     if (!strcmp(cmd, "create")) {
         img_create(argc, argv);
+    } else if (!strcmp(cmd, "check")) {
+        img_check(argc, argv);
     } else if (!strcmp(cmd, "commit")) {
         img_commit(argc, argv);
     } else if (!strcmp(cmd, "convert")) {
-- 
1.6.0.6

[Qemu-devel] [PATCH 4/5] qcow2: Refcount checking code cleanup

[kwolf]

From: Kevin Wolf <kwolf@redhat.com>

This is purely cosmetical changes to make the code easier to read. Move L2
table processing from a deeply nested block to its own function, add some
comments.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block-qcow2.c |  149 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 105 insertions(+), 44 deletions(-)

diff --git a/block-qcow2.c b/block-qcow2.c
index cbadcd0..2e91e6e 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -2603,6 +2603,90 @@ static int inc_refcounts(BlockDriverState *bs,
     return errors;
 }
 
+/*
+ * Increases the refcount in the given refcount table for the all clusters
+ * referenced in the L2 table. While doing so, performs some checks on L2
+ * entries.
+ *
+ * Returns the number of errors found by the checks or -errno if an internal
+ * error occurred.
+ */
+static int check_refcounts_l2(BlockDriverState *bs,
+    uint16_t *refcount_table, int refcount_table_size, int64_t l2_offset,
+    int check_copied)
+{
+    BDRVQcowState *s = bs->opaque;
+    uint64_t *l2_table, offset;
+    int i, l2_size, nb_csectors, refcount;
+    int errors = 0;
+
+    /* Read L2 table from disk */
+    l2_size = s->l2_size * sizeof(uint64_t);
+    l2_table = qemu_malloc(l2_size);
+
+    if (bdrv_pread(s->hd, l2_offset, l2_table, l2_size) != l2_size)
+        goto fail;
+
+    /* Do the actual checks */
+    for(i = 0; i < s->l2_size; i++) {
+        offset = be64_to_cpu(l2_table[i]);
+        if (offset != 0) {
+            if (offset & QCOW_OFLAG_COMPRESSED) {
+                /* Compressed clusters don't have QCOW_OFLAG_COPIED */
+                if (offset & QCOW_OFLAG_COPIED) {
+                    fprintf(stderr, "ERROR: cluster %" PRId64 ": "
+                        "copied flag must never be set for compressed "
+                        "clusters\n", offset >> s->cluster_bits);
+                    offset &= ~QCOW_OFLAG_COPIED;
+                    errors++;
+                }
+
+                /* Mark cluster as used */
+                nb_csectors = ((offset >> s->csize_shift) &
+                               s->csize_mask) + 1;
+                offset &= s->cluster_offset_mask;
+                errors += inc_refcounts(bs, refcount_table,
+                              refcount_table_size,
+                              offset & ~511, nb_csectors * 512);
+            } else {
+                /* QCOW_OFLAG_COPIED must be set iff refcount == 1 */
+                if (check_copied) {
+                    uint64_t entry = offset;
+                    offset &= ~QCOW_OFLAG_COPIED;
+                    refcount = get_refcount(bs, offset) >> s->cluster_bits;
+                    if ((refcount == 1) != ((entry & QCOW_OFLAG_COPIED) != 0)) {
+                        fprintf(stderr, "ERROR OFLAG_COPIED: offset=%"
+                            PRIx64 " refcount=%d\n", entry, refcount);
+                        errors++;
+                    }
+                }
+
+                /* Mark cluster as used */
+                offset &= ~QCOW_OFLAG_COPIED;
+                errors += inc_refcounts(bs, refcount_table,
+                              refcount_table_size,
+                              offset, s->cluster_size);
+            }
+        }
+    }
+
+    qemu_free(l2_table);
+    return errors;
+
+fail:
+    fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n");
+    qemu_free(l2_table);
+    return -EIO;
+}
+
+/*
+ * Increases the refcount for the L1 table, its L2 tables and all referenced
+ * clusters in the given refcount table. While doing so, performs some checks
+ * on L1 and L2 entries.
+ *
+ * Returns the number of errors found by the checks or -errno if an internal
+ * error occurred.
+ */
 static int check_refcounts_l1(BlockDriverState *bs,
                               uint16_t *refcount_table,
                               int refcount_table_size,
@@ -2610,16 +2694,17 @@ static int check_refcounts_l1(BlockDriverState *bs,
                               int check_copied)
 {
     BDRVQcowState *s = bs->opaque;
-    uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2;
-    int l2_size, i, j, nb_csectors, refcount;
+    uint64_t *l1_table, l2_offset, l1_size2;
+    int i, refcount, ret;
     int errors = 0;
 
-    l2_table = NULL;
     l1_size2 = l1_size * sizeof(uint64_t);
 
+    /* Mark L1 table as used */
     errors += inc_refcounts(bs, refcount_table, refcount_table_size,
                   l1_table_offset, l1_size2);
 
+    /* Read L1 table entries from disk */
     l1_table = qemu_malloc(l1_size2);
     if (bdrv_pread(s->hd, l1_table_offset,
                    l1_table, l1_size2) != l1_size2)
@@ -2627,68 +2712,43 @@ static int check_refcounts_l1(BlockDriverState *bs,
     for(i = 0;i < l1_size; i++)
         be64_to_cpus(&l1_table[i]);
 
-    l2_size = s->l2_size * sizeof(uint64_t);
-    l2_table = qemu_malloc(l2_size);
+    /* Do the actual checks */
     for(i = 0; i < l1_size; i++) {
         l2_offset = l1_table[i];
         if (l2_offset) {
+            /* QCOW_OFLAG_COPIED must be set iff refcount == 1 */
             if (check_copied) {
-                refcount = get_refcount(bs, (l2_offset & ~QCOW_OFLAG_COPIED) >> s->cluster_bits);
+                refcount = get_refcount(bs, (l2_offset & ~QCOW_OFLAG_COPIED)
+                    >> s->cluster_bits);
                 if ((refcount == 1) != ((l2_offset & QCOW_OFLAG_COPIED) != 0)) {
                     fprintf(stderr, "ERROR OFLAG_COPIED: l2_offset=%" PRIx64
                         " refcount=%d\n", l2_offset, refcount);
                     errors++;
                 }
             }
+
+            /* Mark L2 table as used */
             l2_offset &= ~QCOW_OFLAG_COPIED;
-            if (bdrv_pread(s->hd, l2_offset, l2_table, l2_size) != l2_size)
-                goto fail;
-            for(j = 0; j < s->l2_size; j++) {
-                offset = be64_to_cpu(l2_table[j]);
-                if (offset != 0) {
-                    if (offset & QCOW_OFLAG_COMPRESSED) {
-                        if (offset & QCOW_OFLAG_COPIED) {
-                            fprintf(stderr, "ERROR: cluster %" PRId64 ": "
-                                "copied flag must never be set for compressed "
-                                "clusters\n", offset >> s->cluster_bits);
-                            offset &= ~QCOW_OFLAG_COPIED;
-                            errors++;
-                        }
-                        nb_csectors = ((offset >> s->csize_shift) &
-                                       s->csize_mask) + 1;
-                        offset &= s->cluster_offset_mask;
-                        errors += inc_refcounts(bs, refcount_table,
-                                      refcount_table_size,
-                                      offset & ~511, nb_csectors * 512);
-                    } else {
-                        if (check_copied) {
-                            refcount = get_refcount(bs, (offset & ~QCOW_OFLAG_COPIED) >> s->cluster_bits);
-                            if ((refcount == 1) != ((offset & QCOW_OFLAG_COPIED) != 0)) {
-                                fprintf(stderr, "ERROR OFLAG_COPIED: offset=%"
-                                    PRIx64 " refcount=%d\n", offset, refcount);
-                                errors++;
-                            }
-                        }
-                        offset &= ~QCOW_OFLAG_COPIED;
-                        errors += inc_refcounts(bs, refcount_table,
-                                      refcount_table_size,
-                                      offset, s->cluster_size);
-                    }
-                }
-            }
             errors += inc_refcounts(bs, refcount_table,
                           refcount_table_size,
                           l2_offset,
                           s->cluster_size);
+
+            /* Process and check L2 entries */
+            ret = check_refcounts_l2(bs, refcount_table, refcount_table_size,
+                l2_offset, check_copied);
+            if (ret < 0) {
+                goto fail;
+            }
+            errors += ret;
         }
     }
     qemu_free(l1_table);
-    qemu_free(l2_table);
     return errors;
- fail:
+
+fail:
     fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n");
     qemu_free(l1_table);
-    qemu_free(l2_table);
     return -EIO;
 }
 
@@ -2715,6 +2775,7 @@ static int check_refcounts(BlockDriverState *bs)
     errors += inc_refcounts(bs, refcount_table, nb_clusters,
                   0, s->cluster_size);
 
+    /* current L1 table */
     ret = check_refcounts_l1(bs, refcount_table, nb_clusters,
                        s->l1_table_offset, s->l1_size, 1);
     if (ret < 0) {
-- 
1.6.0.6

[Qemu-devel] [PATCH 5/5] qcow2: Add plausibility check for L1/L2 entries

[kwolf]

From: Kevin Wolf <kwolf@redhat.com>

All L1 and L2 entries must point at the start of a cluster. If there is some
offset into the cluster, the entry is corrupted.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block-qcow2.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/block-qcow2.c b/block-qcow2.c
index 2e91e6e..25c1d23 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -2666,6 +2666,13 @@ static int check_refcounts_l2(BlockDriverState *bs,
                 errors += inc_refcounts(bs, refcount_table,
                               refcount_table_size,
                               offset, s->cluster_size);
+
+                /* Correct offsets are cluster aligned */
+                if (offset & (s->cluster_size - 1)) {
+                    fprintf(stderr, "ERROR offset=%" PRIx64 ": Cluster is not "
+                        "properly aligned; L2 entry corrupted.\n", offset);
+                    errors++;
+                }
             }
         }
     }
@@ -2734,6 +2741,13 @@ static int check_refcounts_l1(BlockDriverState *bs,
                           l2_offset,
                           s->cluster_size);
 
+            /* L2 tables are cluster aligned */
+            if (l2_offset & (s->cluster_size - 1)) {
+                fprintf(stderr, "ERROR l2_offset=%" PRIx64 ": Table is not "
+                    "cluster aligned; L1 entry corrupted\n", l2_offset);
+                errors++;
+            }
+
             /* Process and check L2 entries */
             ret = check_refcounts_l2(bs, refcount_table, refcount_table_size,
                 l2_offset, check_copied);
-- 
1.6.0.6

Re: [Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Anthony Liguori]

Hi Christoph,

Do you have a qemu-io script handy that can be used to stress something 
like this patch set?  After the last qcow2 regression, I'm wary of 
additional cleanups that we can't validate with a strong stress test.

Regards,

Anthony Liguori

kwolf@redhat.com wrote:
> From: Kevin Wolf <kwolf@redhat.com>
>
> This code is currently only compiled when DEBUG_ALLOC is defined, so you
> usually don't see compiler warnings on it. This patch series wants to enable
> the code, so fix the format string warnings first.
>
> While we're at it, let's print error messages to stderr.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>   


-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Kevin Wolf]

Hi Anthony,

Am Freitag, 17. April 2009 22:39 schrieb Anthony Liguori:
> Do you have a qemu-io script handy that can be used to stress something
> like this patch set?  After the last qcow2 regression, I'm wary of
> additional cleanups that we can't validate with a strong stress test.

This patch series is harmless in that respect. You can tell alone from looking 
at the patches that it can't cause regressions in normal operation, because 
it only touches code which was previosuly not even built and is only called 
by qemu-img (after patch 3) and when DEBUG_ALLOC is defined.

But you would better apply the corruption fix I sent on Wednesday. ;-)

And even though I think that this series can't break anything, we definitely 
could use a strong test suite. I'm almost sure that there is at least one bug 
left (the one Jamie Lokier saw from 5006 on, but nobody ever found it).

Kevin

Re: [Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Anthony Liguori]

Kevin Wolf wrote:
> Hi Anthony,
>
> Am Freitag, 17. April 2009 22:39 schrieb Anthony Liguori:
>   
>> Do you have a qemu-io script handy that can be used to stress something
>> like this patch set?  After the last qcow2 regression, I'm wary of
>> additional cleanups that we can't validate with a strong stress test.
>>     
>
> This patch series is harmless in that respect. You can tell alone from looking 
> at the patches that it can't cause regressions in normal operation, because 
> it only touches code which was previosuly not even built and is only called 
> by qemu-img (after patch 3) and when DEBUG_ALLOC is defined.
>   

I'm basically at the point of not wanting to touch qcow2 without serious 
testing.  That said, I can do enough on my own to satisfy me so I'll 
commit this series later today or tomorrow.

> But you would better apply the corruption fix I sent on Wednesday. ;-)
>   

Yes, I just checked that in.  Very good catch!

> And even though I think that this series can't break anything, we definitely 
> could use a strong test suite. I'm almost sure that there is at least one bug 
> left (the one Jamie Lokier saw from 5006 on, but nobody ever found it).
>   

You don't think that was Nolan's fix?

> Kevin
>   


-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Kevin Wolf]

Am Freitag, 17. April 2009 23:03 schrieb Anthony Liguori:
> Kevin Wolf wrote:
> > Hi Anthony,
> >
> > Am Freitag, 17. April 2009 22:39 schrieb Anthony Liguori:
> >> Do you have a qemu-io script handy that can be used to stress something
> >> like this patch set?  After the last qcow2 regression, I'm wary of
> >> additional cleanups that we can't validate with a strong stress test.
> >
> > This patch series is harmless in that respect. You can tell alone from
> > looking at the patches that it can't cause regressions in normal
> > operation, because it only touches code which was previosuly not even
> > built and is only called by qemu-img (after patch 3) and when DEBUG_ALLOC
> > is defined.
>
> I'm basically at the point of not wanting to touch qcow2 without serious
> testing.  That said, I can do enough on my own to satisfy me so I'll
> commit this series later today or tomorrow.

I perfectly understand that you don't want to break it again. But then, the 
only way to avoid new bugs is to stop development completely. This isn't a 
solution either.

This is even more true for changes which are actually made for testing and 
debugging purposes like these. This series is what helped me to find the 
corruption bug.

What we should do is to make sure that qcow2 patches (especially those 
touching the core) are given a thorough review before committing.

> > But you would better apply the corruption fix I sent on Wednesday. ;-)
>
> Yes, I just checked that in.  Very good catch!
>
> > And even though I think that this series can't break anything, we
> > definitely could use a strong test suite. I'm almost sure that there is
> > at least one bug left (the one Jamie Lokier saw from 5006 on, but nobody
> > ever found it).
>
> You don't think that was Nolan's fix?

Hm, I haven't look very much in detail at it. But according to the commit log 
only qcow_is_allocated() was affected, and I can't see how booting Jamie's 
Windows guest would call this function.

Kevin

Re: [Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Anthony Liguori]

Kevin Wolf wrote:
> Am Freitag, 17. April 2009 23:03 schrieb Anthony Liguori:
>   
>> I'm basically at the point of not wanting to touch qcow2 without serious
>> testing.  That said, I can do enough on my own to satisfy me so I'll
>> commit this series later today or tomorrow.
>>     
>
> I perfectly understand that you don't want to break it again. But then, the 
> only way to avoid new bugs is to stop development completely. This isn't a 
> solution either.
>   

Every patch I commit gets tested.  I have various tests that I run 
depending on which subsystem the patch touches.  Right now, for qcow2, I 
don't have nearly enough.  I was hoping that Christoph had something 
laying around that I could use since it looks like qemu-io would be a 
great harness for qcow2 changes.

But I can write a pretty easy script myself on top of qemu-io so it's no 
big deal.  I'm not suggesting holding up development.

> This is even more true for changes which are actually made for testing and 
> debugging purposes like these. This series is what helped me to find the 
> corruption bug.
>
> What we should do is to make sure that qcow2 patches (especially those 
> touching the core) are given a thorough review before committing.
>   

In theory, r5006 did.  That wasn't enough.

>>> And even though I think that this series can't break anything, we
>>> definitely could use a strong test suite. I'm almost sure that there is
>>> at least one bug left (the one Jamie Lokier saw from 5006 on, but nobody
>>> ever found it).
>>>       
>> You don't think that was Nolan's fix?
>>     
>
> Hm, I haven't look very much in detail at it. But according to the commit log 
> only qcow_is_allocated() was affected, and I can't see how booting Jamie's 
> Windows guest would call this function.
>   

The bug was in get_cluster_offset() so it could have caused much more 
subtle breakages.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Kevin Wolf]

Am Samstag, 18. April 2009 00:07 schrieben Sie:
> Every patch I commit gets tested.  I have various tests that I run
> depending on which subsystem the patch touches.  Right now, for qcow2, I
> don't have nearly enough.  I was hoping that Christoph had something
> laying around that I could use since it looks like qemu-io would be a
> great harness for qcow2 changes.

It is, definitely. qemu-io is the other part which I needed. But qemu-io alone 
won't make you happy. It can exercise a lot of code, but it can't tell you if 
your image is broken in the end. Except for reading back written data, of 
course, but breakage is often enough more subtle.

> But I can write a pretty easy script myself on top of qemu-io so it's no
> big deal.  I'm not suggesting holding up development.

Heh, so everyone is writing his own scripts. What would you think about 
bringing the test cases you use into the repository?

> > This is even more true for changes which are actually made for testing
> > and debugging purposes like these. This series is what helped me to find
> > the corruption bug.
> >
> > What we should do is to make sure that qcow2 patches (especially those
> > touching the core) are given a thorough review before committing.
>
> In theory, r5006 did.  That wasn't enough.

I know. It also was tested a fair amount and it wasn't enough. We need both, 
but currently we don't have any systematic tests for it.

> >>> And even though I think that this series can't break anything, we
> >>> definitely could use a strong test suite. I'm almost sure that there is
> >>> at least one bug left (the one Jamie Lokier saw from 5006 on, but
> >>> nobody ever found it).
> >>
> >> You don't think that was Nolan's fix?
> >
> > Hm, I haven't look very much in detail at it. But according to the commit
> > log only qcow_is_allocated() was affected, and I can't see how booting
> > Jamie's Windows guest would call this function.
>
> The bug was in get_cluster_offset() so it could have caused much more
> subtle breakages.

Maybe. I wouldn't count on that bug being fixed, but it might be me who is 
overly skeptical.

Kevin

Re: [Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Anthony Liguori]

Kevin Wolf wrote:
> Am Samstag, 18. April 2009 00:07 schrieben Sie:
>   
>> Every patch I commit gets tested.  I have various tests that I run
>> depending on which subsystem the patch touches.  Right now, for qcow2, I
>> don't have nearly enough.  I was hoping that Christoph had something
>> laying around that I could use since it looks like qemu-io would be a
>> great harness for qcow2 changes.
>>     
>
> It is, definitely. qemu-io is the other part which I needed. But qemu-io alone 
> won't make you happy. It can exercise a lot of code, but it can't tell you if 
> your image is broken in the end. Except for reading back written data, of 
> course, but breakage is often enough more subtle.
>
>   
>> But I can write a pretty easy script myself on top of qemu-io so it's no
>> big deal.  I'm not suggesting holding up development.
>>     
>
> Heh, so everyone is writing his own scripts. What would you think about 
> bringing the test cases you use into the repository?
>   

Yeah, I've been wanting to do everything via kvm-autotest but so far, 
it's not quite up to what I want to use it for.  I'll see about cleaning 
up some of my scripts and posting them.  The problem is they rely on 
custom images.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Ryan Harper]

* Anthony Liguori <aliguori@us.ibm.com> [2009-04-17 17:38]:
> Kevin Wolf wrote:
> >Am Samstag, 18. April 2009 00:07 schrieben Sie:
> >  
> >>Every patch I commit gets tested.  I have various tests that I run
> >>depending on which subsystem the patch touches.  Right now, for qcow2, I
> >>don't have nearly enough.  I was hoping that Christoph had something
> >>laying around that I could use since it looks like qemu-io would be a
> >>great harness for qcow2 changes.
> >>    
> >
> >It is, definitely. qemu-io is the other part which I needed. But qemu-io 
> >alone won't make you happy. It can exercise a lot of code, but it can't 
> >tell you if your image is broken in the end. Except for reading back 
> >written data, of course, but breakage is often enough more subtle.
> >
> >  
> >>But I can write a pretty easy script myself on top of qemu-io so it's no
> >>big deal.  I'm not suggesting holding up development.
> >>    
> >
> >Heh, so everyone is writing his own scripts. What would you think about 
> >bringing the test cases you use into the repository?
> >  
> 
> Yeah, I've been wanting to do everything via kvm-autotest but so far, 
> it's not quite up to what I want to use it for.  I'll see about cleaning 
> up some of my scripts and posting them.  The problem is they rely on 
> custom images.

If you post em, I'm sure we can work it in.


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

Re: [Qemu-devel] [PATCH 0/5] Add qemu-img check subcommand

[Gleb Natapov]

On Fri, Apr 17, 2009 at 02:04:34PM +0200, Kevin Wolf wrote:
> Current qcow2 code already includes some functions to do some consistency
> checks (e.g. compare refcounts to real usage). Allowing the user (or developer
> debugging qcow2 code) to check his images only involves actually building this
> code and exposing it to the user by qemu-img.
> 
> This patch series implements a check subcommand for qemu-img. I think it will
> be useful especially in combination with qemu-io for test suites.
> 
Qumranet's product had this functionality and I found it to be so useful
that I still have qemu-img binary that has this "check" verb. For
instance if windows starts to BSOD to often the first thing I do is to
check that image is not corrupted.

> Kevin
> 
> Kevin Wolf (5):
>   qcow2: Fix warnings in check_refcount()
>   Introduce bdrv_check
>   Introduce qemu-img check subcommand
>   qcow2: Refcount checking code cleanup
>   qcow2: Add plausibility check for L1/L2 entries
> 
>  block-qcow2.c |  239 +++++++++++++++++++++++++++++++++++++++++---------------
>  block.c       |   14 ++++
>  block.h       |    1 +
>  block_int.h   |    3 +
>  qemu-img.c    |   62 +++++++++++++++
>  5 files changed, 255 insertions(+), 64 deletions(-)
> 
> 

--
			Gleb.

Re: [Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Christoph Hellwig]

On Fri, Apr 17, 2009 at 03:39:03PM -0500, Anthony Liguori wrote:
> Hi Christoph,
> 
> Do you have a qemu-io script handy that can be used to stress something 
> like this patch set?  After the last qcow2 regression, I'm wary of 
> additional cleanups that we can't validate with a strong stress test.

I only have a couple of basic tests so far, and currently I'm looking
through the ML archives for all kinds of test cases.  Kevin has a copy
of the current state which I don't want to release yet before some
licensing bits are clarified.

Re: [Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Christoph Hellwig]

On Fri, Apr 17, 2009 at 05:31:50PM -0500, Anthony Liguori wrote:
> Yeah, I've been wanting to do everything via kvm-autotest but so far, 
> it's not quite up to what I want to use it for.  I'll see about cleaning 
> up some of my scripts and posting them.  The problem is they rely on 
> custom images.

Having a repository of "problem" or just interesting images that we can
use qemu-io or just plain guests on would be extremty helpful for
extending test coverage.

[Qemu-devel] [PATCH v2 4/5] qcow2: Refcount checking code cleanup

[Kevin Wolf]

This is purely cosmetical changes to make the code easier to read. Move L2
table processing from a deeply nested block to its own function, add some
comments.

Patch v2: Fix misplaced bracket causing false positives

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block-qcow2.c |  149 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 105 insertions(+), 44 deletions(-)

diff --git a/block-qcow2.c b/block-qcow2.c
index cbadcd0..eacac4d 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -2603,6 +2603,90 @@ static int inc_refcounts(BlockDriverState *bs,
     return errors;
 }
 
+/*
+ * Increases the refcount in the given refcount table for the all clusters
+ * referenced in the L2 table. While doing so, performs some checks on L2
+ * entries.
+ *
+ * Returns the number of errors found by the checks or -errno if an internal
+ * error occurred.
+ */
+static int check_refcounts_l2(BlockDriverState *bs,
+    uint16_t *refcount_table, int refcount_table_size, int64_t l2_offset,
+    int check_copied)
+{
+    BDRVQcowState *s = bs->opaque;
+    uint64_t *l2_table, offset;
+    int i, l2_size, nb_csectors, refcount;
+    int errors = 0;
+
+    /* Read L2 table from disk */
+    l2_size = s->l2_size * sizeof(uint64_t);
+    l2_table = qemu_malloc(l2_size);
+
+    if (bdrv_pread(s->hd, l2_offset, l2_table, l2_size) != l2_size)
+        goto fail;
+
+    /* Do the actual checks */
+    for(i = 0; i < s->l2_size; i++) {
+        offset = be64_to_cpu(l2_table[i]);
+        if (offset != 0) {
+            if (offset & QCOW_OFLAG_COMPRESSED) {
+                /* Compressed clusters don't have QCOW_OFLAG_COPIED */
+                if (offset & QCOW_OFLAG_COPIED) {
+                    fprintf(stderr, "ERROR: cluster %" PRId64 ": "
+                        "copied flag must never be set for compressed "
+                        "clusters\n", offset >> s->cluster_bits);
+                    offset &= ~QCOW_OFLAG_COPIED;
+                    errors++;
+                }
+
+                /* Mark cluster as used */
+                nb_csectors = ((offset >> s->csize_shift) &
+                               s->csize_mask) + 1;
+                offset &= s->cluster_offset_mask;
+                errors += inc_refcounts(bs, refcount_table,
+                              refcount_table_size,
+                              offset & ~511, nb_csectors * 512);
+            } else {
+                /* QCOW_OFLAG_COPIED must be set iff refcount == 1 */
+                if (check_copied) {
+                    uint64_t entry = offset;
+                    offset &= ~QCOW_OFLAG_COPIED;
+                    refcount = get_refcount(bs, offset >> s->cluster_bits);
+                    if ((refcount == 1) != ((entry & QCOW_OFLAG_COPIED) != 0)) {
+                        fprintf(stderr, "ERROR OFLAG_COPIED: offset=%"
+                            PRIx64 " refcount=%d\n", entry, refcount);
+                        errors++;
+                    }
+                }
+
+                /* Mark cluster as used */
+                offset &= ~QCOW_OFLAG_COPIED;
+                errors += inc_refcounts(bs, refcount_table,
+                              refcount_table_size,
+                              offset, s->cluster_size);
+            }
+        }
+    }
+
+    qemu_free(l2_table);
+    return errors;
+
+fail:
+    fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n");
+    qemu_free(l2_table);
+    return -EIO;
+}
+
+/*
+ * Increases the refcount for the L1 table, its L2 tables and all referenced
+ * clusters in the given refcount table. While doing so, performs some checks
+ * on L1 and L2 entries.
+ *
+ * Returns the number of errors found by the checks or -errno if an internal
+ * error occurred.
+ */
 static int check_refcounts_l1(BlockDriverState *bs,
                               uint16_t *refcount_table,
                               int refcount_table_size,
@@ -2610,16 +2694,17 @@ static int check_refcounts_l1(BlockDriverState *bs,
                               int check_copied)
 {
     BDRVQcowState *s = bs->opaque;
-    uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2;
-    int l2_size, i, j, nb_csectors, refcount;
+    uint64_t *l1_table, l2_offset, l1_size2;
+    int i, refcount, ret;
     int errors = 0;
 
-    l2_table = NULL;
     l1_size2 = l1_size * sizeof(uint64_t);
 
+    /* Mark L1 table as used */
     errors += inc_refcounts(bs, refcount_table, refcount_table_size,
                   l1_table_offset, l1_size2);
 
+    /* Read L1 table entries from disk */
     l1_table = qemu_malloc(l1_size2);
     if (bdrv_pread(s->hd, l1_table_offset,
                    l1_table, l1_size2) != l1_size2)
@@ -2627,68 +2712,43 @@ static int check_refcounts_l1(BlockDriverState *bs,
     for(i = 0;i < l1_size; i++)
         be64_to_cpus(&l1_table[i]);
 
-    l2_size = s->l2_size * sizeof(uint64_t);
-    l2_table = qemu_malloc(l2_size);
+    /* Do the actual checks */
     for(i = 0; i < l1_size; i++) {
         l2_offset = l1_table[i];
         if (l2_offset) {
+            /* QCOW_OFLAG_COPIED must be set iff refcount == 1 */
             if (check_copied) {
-                refcount = get_refcount(bs, (l2_offset & ~QCOW_OFLAG_COPIED) >> s->cluster_bits);
+                refcount = get_refcount(bs, (l2_offset & ~QCOW_OFLAG_COPIED)
+                    >> s->cluster_bits);
                 if ((refcount == 1) != ((l2_offset & QCOW_OFLAG_COPIED) != 0)) {
                     fprintf(stderr, "ERROR OFLAG_COPIED: l2_offset=%" PRIx64
                         " refcount=%d\n", l2_offset, refcount);
                     errors++;
                 }
             }
+
+            /* Mark L2 table as used */
             l2_offset &= ~QCOW_OFLAG_COPIED;
-            if (bdrv_pread(s->hd, l2_offset, l2_table, l2_size) != l2_size)
-                goto fail;
-            for(j = 0; j < s->l2_size; j++) {
-                offset = be64_to_cpu(l2_table[j]);
-                if (offset != 0) {
-                    if (offset & QCOW_OFLAG_COMPRESSED) {
-                        if (offset & QCOW_OFLAG_COPIED) {
-                            fprintf(stderr, "ERROR: cluster %" PRId64 ": "
-                                "copied flag must never be set for compressed "
-                                "clusters\n", offset >> s->cluster_bits);
-                            offset &= ~QCOW_OFLAG_COPIED;
-                            errors++;
-                        }
-                        nb_csectors = ((offset >> s->csize_shift) &
-                                       s->csize_mask) + 1;
-                        offset &= s->cluster_offset_mask;
-                        errors += inc_refcounts(bs, refcount_table,
-                                      refcount_table_size,
-                                      offset & ~511, nb_csectors * 512);
-                    } else {
-                        if (check_copied) {
-                            refcount = get_refcount(bs, (offset & ~QCOW_OFLAG_COPIED) >> s->cluster_bits);
-                            if ((refcount == 1) != ((offset & QCOW_OFLAG_COPIED) != 0)) {
-                                fprintf(stderr, "ERROR OFLAG_COPIED: offset=%"
-                                    PRIx64 " refcount=%d\n", offset, refcount);
-                                errors++;
-                            }
-                        }
-                        offset &= ~QCOW_OFLAG_COPIED;
-                        errors += inc_refcounts(bs, refcount_table,
-                                      refcount_table_size,
-                                      offset, s->cluster_size);
-                    }
-                }
-            }
             errors += inc_refcounts(bs, refcount_table,
                           refcount_table_size,
                           l2_offset,
                           s->cluster_size);
+
+            /* Process and check L2 entries */
+            ret = check_refcounts_l2(bs, refcount_table, refcount_table_size,
+                l2_offset, check_copied);
+            if (ret < 0) {
+                goto fail;
+            }
+            errors += ret;
         }
     }
     qemu_free(l1_table);
-    qemu_free(l2_table);
     return errors;
- fail:
+
+fail:
     fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n");
     qemu_free(l1_table);
-    qemu_free(l2_table);
     return -EIO;
 }
 
@@ -2715,6 +2775,7 @@ static int check_refcounts(BlockDriverState *bs)
     errors += inc_refcounts(bs, refcount_table, nb_clusters,
                   0, s->cluster_size);
 
+    /* current L1 table */
     ret = check_refcounts_l1(bs, refcount_table, nb_clusters,
                        s->l1_table_offset, s->l1_size, 1);
     if (ret < 0) {
-- 
1.6.0.6

Re: [Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Kevin Wolf]

Anthony Liguori schrieb:
> Kevin Wolf wrote:
>> And even though I think that this series can't break anything, we 
>> definitely could use a strong test suite. I'm almost sure that there 
>> is at least one bug left (the one Jamie Lokier saw from 5006 on, but 
>> nobody ever found it).
>>   
> 
> You don't think that was Nolan's fix?

I've finally looked at the code longer than ten seconds and noticed that 
a) I misunderstood the patch comment and b) the code looks different 
than I seemed to remember. So I can live now with the assumption that it 
was what Nolan's fix addresses, doesn't seem too unlikely.

Kevin

Re: [Qemu-devel] [PATCH 1/5] qcow2: Fix warnings in check_refcount()

[Anthony Liguori]

kwolf@redhat.com wrote:
> From: Kevin Wolf <kwolf@redhat.com>
>
> This code is currently only compiled when DEBUG_ALLOC is defined, so you
> usually don't see compiler warnings on it. This patch series wants to enable
> the code, so fix the format string warnings first.
>
> While we're at it, let's print error messages to stderr.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>   
Applied.  Thanks.

-- 
Regards,

Anthony Liguori