diff mbox series

[for-4.2,06/13] qcow2: Separate qcow2_check_read_snapshot_table()

Message ID 20190730172508.19911-7-mreitz@redhat.com (mailing list archive)
State New, archived
Headers show
Series qcow2: Let check -r all repair some snapshot bits | expand

Commit Message

Max Reitz July 30, 2019, 5:25 p.m. UTC
Reading the snapshot table can fail.  That is a problem when we want to
repair the image.

Therefore, stop reading the snapshot table in qcow2_do_open() in check
mode.  Instead, add a new function qcow2_check_read_snapshot_table()
that reads the snapshot table at a later point.  In the future, we want
to handle errors here and fix them.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.h          |  4 +++
 block/qcow2-snapshot.c | 58 ++++++++++++++++++++++++++++++++
 block/qcow2.c          | 76 ++++++++++++++++++++++++++++++++----------
 3 files changed, 120 insertions(+), 18 deletions(-)

Comments

Eric Blake July 30, 2019, 6:53 p.m. UTC | #1
On 7/30/19 12:25 PM, Max Reitz wrote:
> Reading the snapshot table can fail.  That is a problem when we want to
> repair the image.
> 
> Therefore, stop reading the snapshot table in qcow2_do_open() in check
> mode.  Instead, add a new function qcow2_check_read_snapshot_table()
> that reads the snapshot table at a later point.  In the future, we want
> to handle errors here and fix them.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.h          |  4 +++
>  block/qcow2-snapshot.c | 58 ++++++++++++++++++++++++++++++++
>  block/qcow2.c          | 76 ++++++++++++++++++++++++++++++++----------
>  3 files changed, 120 insertions(+), 18 deletions(-)
> 

> +++ b/block/qcow2-snapshot.c
> @@ -321,6 +321,64 @@ fail:
>      return ret;
>  }
>  
> +int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
> +                                                 BdrvCheckResult *result,
> +                                                 BdrvCheckMode fix)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    Error *local_err = NULL;
> +    int ret;
> +    struct {
> +        uint32_t nb_snapshots;
> +        uint64_t snapshots_offset;
> +    } QEMU_PACKED snapshot_table_pointer;
> +
> +    /* qcow2_do_open() discards this information in check mode */
> +    ret = bdrv_pread(bs->file, 60, &snapshot_table_pointer,
> +                     sizeof(snapshot_table_pointer));

Should that '60' be a named constant or offsetof() expression?  (I know,
you just copied this instance from elsewhere)

Reviewed-by: Eric Blake <eblake@redhat.com>
Max Reitz July 31, 2019, 8:59 a.m. UTC | #2
On 30.07.19 20:53, Eric Blake wrote:
> On 7/30/19 12:25 PM, Max Reitz wrote:
>> Reading the snapshot table can fail.  That is a problem when we want to
>> repair the image.
>>
>> Therefore, stop reading the snapshot table in qcow2_do_open() in check
>> mode.  Instead, add a new function qcow2_check_read_snapshot_table()
>> that reads the snapshot table at a later point.  In the future, we want
>> to handle errors here and fix them.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/qcow2.h          |  4 +++
>>  block/qcow2-snapshot.c | 58 ++++++++++++++++++++++++++++++++
>>  block/qcow2.c          | 76 ++++++++++++++++++++++++++++++++----------
>>  3 files changed, 120 insertions(+), 18 deletions(-)
>>
> 
>> +++ b/block/qcow2-snapshot.c
>> @@ -321,6 +321,64 @@ fail:
>>      return ret;
>>  }
>>  
>> +int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
>> +                                                 BdrvCheckResult *result,
>> +                                                 BdrvCheckMode fix)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +    Error *local_err = NULL;
>> +    int ret;
>> +    struct {
>> +        uint32_t nb_snapshots;
>> +        uint64_t snapshots_offset;
>> +    } QEMU_PACKED snapshot_table_pointer;
>> +
>> +    /* qcow2_do_open() discards this information in check mode */
>> +    ret = bdrv_pread(bs->file, 60, &snapshot_table_pointer,
>> +                     sizeof(snapshot_table_pointer));
> 
> Should that '60' be a named constant or offsetof() expression?  (I know,
> you just copied this instance from elsewhere)

Well, I copied it from the specification. O:-)

You’re completely right.  It should be offsetof(QCowHeader, nb_snapshots).

(I blame the fact that I had started writing the test by this point, so
I was already immersed in so many magic numbers.)

> Reviewed-by: Eric Blake <eblake@redhat.com>
diff mbox series

Patch

diff --git a/block/qcow2.h b/block/qcow2.h
index 77586d81b9..50c7eefb5b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -712,6 +712,10 @@  void qcow2_free_snapshots(BlockDriverState *bs);
 int qcow2_read_snapshots(BlockDriverState *bs, Error **errp);
 int qcow2_write_snapshots(BlockDriverState *bs);
 
+int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
+                                                 BdrvCheckResult *result,
+                                                 BdrvCheckMode fix);
+
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
                                unsigned table_size);
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 3d097b92d7..cbc6d699b6 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -321,6 +321,64 @@  fail:
     return ret;
 }
 
+int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
+                                                 BdrvCheckResult *result,
+                                                 BdrvCheckMode fix)
+{
+    BDRVQcow2State *s = bs->opaque;
+    Error *local_err = NULL;
+    int ret;
+    struct {
+        uint32_t nb_snapshots;
+        uint64_t snapshots_offset;
+    } QEMU_PACKED snapshot_table_pointer;
+
+    /* qcow2_do_open() discards this information in check mode */
+    ret = bdrv_pread(bs->file, 60, &snapshot_table_pointer,
+                     sizeof(snapshot_table_pointer));
+    if (ret < 0) {
+        result->check_errors++;
+        fprintf(stderr, "ERROR failed to read the snapshot table pointer from "
+                "the image header: %s\n", strerror(-ret));
+        return ret;
+    }
+
+    s->snapshots_offset = be64_to_cpu(snapshot_table_pointer.snapshots_offset);
+    s->nb_snapshots = be32_to_cpu(snapshot_table_pointer.nb_snapshots);
+
+    ret = qcow2_validate_table(bs, s->snapshots_offset, s->nb_snapshots,
+                               sizeof(QCowSnapshotHeader),
+                               sizeof(QCowSnapshotHeader) * QCOW_MAX_SNAPSHOTS,
+                               "snapshot table", &local_err);
+    if (ret < 0) {
+        result->check_errors++;
+        error_reportf_err(local_err, "ERROR ");
+
+        /* We did not read the snapshot table, so invalidate this information */
+        s->snapshots_offset = 0;
+        s->nb_snapshots = 0;
+
+        return ret;
+    }
+
+    qemu_co_mutex_unlock(&s->lock);
+    ret = qcow2_read_snapshots(bs, &local_err);
+    qemu_co_mutex_lock(&s->lock);
+    if (ret < 0) {
+        result->check_errors++;
+        error_reportf_err(local_err,
+                          "ERROR failed to read the snapshot table: ");
+
+        /* We did not read the snapshot table, so invalidate this information */
+        s->snapshots_offset = 0;
+        s->nb_snapshots = 0;
+
+        return ret;
+    }
+
+    return 0;
+}
+
 static void find_new_snapshot_id(BlockDriverState *bs,
                                  char *id_str, int id_str_size)
 {
diff --git a/block/qcow2.c b/block/qcow2.c
index 75d41683a1..2983a7795a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -567,11 +567,40 @@  int qcow2_mark_consistent(BlockDriverState *bs)
     return 0;
 }
 
+static void qcow2_add_check_result(BdrvCheckResult *out,
+                                   const BdrvCheckResult *src,
+                                   bool set_allocation_info)
+{
+    out->corruptions += src->corruptions;
+    out->leaks += src->leaks;
+    out->check_errors += src->check_errors;
+    out->corruptions_fixed += src->corruptions_fixed;
+    out->leaks_fixed += src->leaks_fixed;
+
+    if (set_allocation_info) {
+        out->image_end_offset = src->image_end_offset;
+        out->bfi = src->bfi;
+    }
+}
+
 static int coroutine_fn qcow2_co_check_locked(BlockDriverState *bs,
                                               BdrvCheckResult *result,
                                               BdrvCheckMode fix)
 {
-    int ret = qcow2_check_refcounts(bs, result, fix);
+    BdrvCheckResult snapshot_res = {};
+    BdrvCheckResult refcount_res = {};
+    int ret;
+
+    memset(result, 0, sizeof(*result));
+
+    ret = qcow2_check_read_snapshot_table(bs, &snapshot_res, fix);
+    qcow2_add_check_result(result, &snapshot_res, false);
+    if (ret < 0) {
+        return ret;
+    }
+
+    ret = qcow2_check_refcounts(bs, &refcount_res, fix);
+    qcow2_add_check_result(result, &refcount_res, true);
     if (ret < 0) {
         return ret;
     }
@@ -1403,17 +1432,22 @@  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
         goto fail;
     }
 
-    /* The total size in bytes of the snapshot table is checked in
-     * qcow2_read_snapshots() because the size of each snapshot is
-     * variable and we don't know it yet.
-     * Here we only check the offset and number of snapshots. */
-    ret = qcow2_validate_table(bs, header.snapshots_offset,
-                               header.nb_snapshots,
-                               sizeof(QCowSnapshotHeader),
-                               sizeof(QCowSnapshotHeader) * QCOW_MAX_SNAPSHOTS,
-                               "Snapshot table", errp);
-    if (ret < 0) {
-        goto fail;
+    if (!(flags & BDRV_O_CHECK)) {
+        /*
+         * The total size in bytes of the snapshot table is checked in
+         * qcow2_read_snapshots() because the size of each snapshot is
+         * variable and we don't know it yet.
+         * Here we only check the offset and number of snapshots.
+         */
+        ret = qcow2_validate_table(bs, header.snapshots_offset,
+                                   header.nb_snapshots,
+                                   sizeof(QCowSnapshotHeader),
+                                   sizeof(QCowSnapshotHeader) *
+                                       QCOW_MAX_SNAPSHOTS,
+                                   "Snapshot table", errp);
+        if (ret < 0) {
+            goto fail;
+        }
     }
 
     /* read the level 1 table */
@@ -1573,13 +1607,19 @@  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
         s->image_backing_file = g_strdup(bs->auto_backing_file);
     }
 
-    /* Internal snapshots */
-    s->snapshots_offset = header.snapshots_offset;
-    s->nb_snapshots = header.nb_snapshots;
+    /*
+     * Internal snapshots; skip reading them in check mode, because
+     * we do not need them then, and we do not want to abort because
+     * of a broken table.
+     */
+    if (!(flags & BDRV_O_CHECK)) {
+        s->snapshots_offset = header.snapshots_offset;
+        s->nb_snapshots = header.nb_snapshots;
 
-    ret = qcow2_read_snapshots(bs, errp);
-    if (ret < 0) {
-        goto fail;
+        ret = qcow2_read_snapshots(bs, errp);
+        if (ret < 0) {
+            goto fail;
+        }
     }
 
     /* Clear unknown autoclear feature bits */