[Qemu-devel] [PATCH 0/2] qcow2: Rework header updates

[Qemu-devel] [PATCH 0/2] qcow2: Rework header updates

[Kevin Wolf]

This series does not only fix the chance of breaking images with a crash in the
middle of a backing file link update, but also fixes header updates in images
with unknown header extensions. It provides functions to update the whole
header instead of only the header extensions.

These fixes and improvements are a prerequisite for any qcow3 code.

Kevin Wolf (2):
  qcow2: Update whole header at once
  qcow2: Keep unknown header extension when rewriting header

 block/qcow2.c |  194 +++++++++++++++++++++++++++++++++++++++------------------
 block/qcow2.h |    9 +++
 2 files changed, 143 insertions(+), 60 deletions(-)

-- 
1.7.6.5

[Qemu-devel] [PATCH 1/2] qcow2: Update whole header at once

[Kevin Wolf]

In order to switch the backing file, qcow2 issues multiple write
requests that only changed a part of the image header. Any failure after
the first one would leave the header in an corrupted state. With this
patch, the whole header is written at once, so we can't fail in the
middle.

At the same time, this gives us a reusable functions that updates all
fields of the qcow2 header and not only the backing file.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c |  154 +++++++++++++++++++++++++++++++++++----------------------
 block/qcow2.h |    1 +
 2 files changed, 95 insertions(+), 60 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index aa32e8d..9f8c2de 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -669,103 +669,137 @@ static void qcow2_invalidate_cache(BlockDriverState *bs)
     }
 }
 
+static size_t header_ext_add(char *buf, uint32_t magic, const void *s,
+    size_t len, size_t buflen)
+{
+    QCowExtension *ext_backing_fmt = (QCowExtension*) buf;
+    size_t ext_len = sizeof(QCowExtension) + ((len + 7) & ~7);
+
+    if (buflen < ext_len) {
+        return -ENOSPC;
+    }
+
+    *ext_backing_fmt = (QCowExtension) {
+        .magic  = cpu_to_be32(magic),
+        .len    = cpu_to_be32(len),
+    };
+    memcpy(buf + sizeof(QCowExtension), s, len);
+
+    return ext_len;
+}
+
 /*
- * Updates the variable length parts of the qcow2 header, i.e. the backing file
- * name and all extensions. qcow2 was not designed to allow such changes, so if
- * we run out of space (we can only use the first cluster) this function may
- * fail.
+ * Updates the qcow2 header, including the variable length parts of it, i.e.
+ * the backing file name and all extensions. qcow2 was not designed to allow
+ * such changes, so if we run out of space (we can only use the first cluster)
+ * this function may fail.
  *
  * Returns 0 on success, -errno in error cases.
  */
-static int qcow2_update_ext_header(BlockDriverState *bs,
-    const char *backing_file, const char *backing_fmt)
+int qcow2_update_header(BlockDriverState *bs)
 {
-    size_t backing_file_len = 0;
-    size_t backing_fmt_len = 0;
     BDRVQcowState *s = bs->opaque;
-    QCowExtension ext_backing_fmt = {0, 0};
+    QCowHeader *header;
+    char *buf;
+    size_t buflen = s->cluster_size;
     int ret;
+    uint64_t total_size;
+    uint32_t refcount_table_clusters;
 
-    /* Backing file format doesn't make sense without a backing file */
-    if (backing_fmt && !backing_file) {
-        return -EINVAL;
-    }
-
-    /* Prepare the backing file format extension if needed */
-    if (backing_fmt) {
-        ext_backing_fmt.len = cpu_to_be32(strlen(backing_fmt));
-        ext_backing_fmt.magic = cpu_to_be32(QCOW2_EXT_MAGIC_BACKING_FORMAT);
-        backing_fmt_len = ((sizeof(ext_backing_fmt)
-            + strlen(backing_fmt) + 7) & ~7);
-    }
+    buf = qemu_blockalign(bs, buflen);
+    memset(buf, 0, s->cluster_size);
 
-    /* Check if we can fit the new header into the first cluster */
-    if (backing_file) {
-        backing_file_len = strlen(backing_file);
-    }
+    /* Header structure */
+    header = (QCowHeader*) buf;
 
-    size_t header_size = sizeof(QCowHeader) + backing_file_len
-        + backing_fmt_len;
-
-    if (header_size > s->cluster_size) {
-        return -ENOSPC;
+    if (buflen < sizeof(*header)) {
+        ret = -ENOSPC;
+        goto fail;
     }
 
-    /* Rewrite backing file name and qcow2 extensions */
-    size_t ext_size = header_size - sizeof(QCowHeader);
-    uint8_t buf[ext_size];
-    size_t offset = 0;
-    size_t backing_file_offset = 0;
-
-    if (backing_file) {
-        if (backing_fmt) {
-            int padding = backing_fmt_len -
-                (sizeof(ext_backing_fmt) + strlen(backing_fmt));
-
-            memcpy(buf + offset, &ext_backing_fmt, sizeof(ext_backing_fmt));
-            offset += sizeof(ext_backing_fmt);
+    total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
+    refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 3);
+
+    *header = (QCowHeader) {
+        .magic                  = cpu_to_be32(QCOW_MAGIC),
+        .version                = cpu_to_be32(QCOW_VERSION),
+        .backing_file_offset    = 0,
+        .backing_file_size      = 0,
+        .cluster_bits           = cpu_to_be32(s->cluster_bits),
+        .size                   = cpu_to_be64(total_size),
+        .crypt_method           = cpu_to_be32(s->crypt_method_header),
+        .l1_size                = cpu_to_be32(s->l1_size),
+        .l1_table_offset        = cpu_to_be64(s->l1_table_offset),
+        .refcount_table_offset  = cpu_to_be64(s->refcount_table_offset),
+        .refcount_table_clusters = cpu_to_be32(refcount_table_clusters),
+        .nb_snapshots           = cpu_to_be32(s->nb_snapshots),
+        .snapshots_offset       = cpu_to_be64(s->snapshots_offset),
+    };
 
-            memcpy(buf + offset, backing_fmt, strlen(backing_fmt));
-            offset += strlen(backing_fmt);
+    buf += sizeof(*header);
+    buflen -= sizeof(*header);
 
-            memset(buf + offset, 0, padding);
-            offset += padding;
+    /* Backing file format header extension */
+    if (*bs->backing_format) {
+        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_BACKING_FORMAT,
+                             bs->backing_format, strlen(bs->backing_format),
+                             buflen);
+        if (ret < 0) {
+            goto fail;
         }
 
-        memcpy(buf + offset, backing_file, backing_file_len);
-        backing_file_offset = sizeof(QCowHeader) + offset;
+        buf += ret;
+        buflen -= ret;
     }
 
-    ret = bdrv_pwrite_sync(bs->file, sizeof(QCowHeader), buf, ext_size);
+    /* End of header extensions */
+    ret = header_ext_add(buf, QCOW2_EXT_MAGIC_END, NULL, 0, buflen);
     if (ret < 0) {
         goto fail;
     }
 
-    /* Update header fields */
-    uint64_t be_backing_file_offset = cpu_to_be64(backing_file_offset);
-    uint32_t be_backing_file_size = cpu_to_be32(backing_file_len);
+    buf += ret;
+    buflen -= ret;
 
-    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, backing_file_offset),
-        &be_backing_file_offset, sizeof(uint64_t));
-    if (ret < 0) {
-        goto fail;
+    /* Backing file name */
+    if (*bs->backing_file) {
+        size_t backing_file_len = strlen(bs->backing_file);
+
+        if (buflen < backing_file_len) {
+            ret = -ENOSPC;
+            goto fail;
+        }
+
+        strncpy(buf, bs->backing_file, buflen);
+
+        header->backing_file_offset = cpu_to_be64(buf - ((char*) header));
+        header->backing_file_size   = cpu_to_be32(backing_file_len);
     }
 
-    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, backing_file_size),
-        &be_backing_file_size, sizeof(uint32_t));
+    /* Write the new header */
+    ret = bdrv_pwrite(bs->file, 0, header, s->cluster_size);
     if (ret < 0) {
         goto fail;
     }
 
     ret = 0;
 fail:
+    qemu_vfree(header);
     return ret;
 }
 
 static int qcow2_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt)
 {
-    return qcow2_update_ext_header(bs, backing_file, backing_fmt);
+    /* Backing file format doesn't make sense without a backing file */
+    if (backing_fmt && !backing_file) {
+        return -EINVAL;
+    }
+
+    pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_file ?: "");
+    pstrcpy(bs->backing_format, sizeof(bs->backing_format), backing_fmt ?: "");
+
+    return qcow2_update_header(bs);
 }
 
 static int preallocate(BlockDriverState *bs)
diff --git a/block/qcow2.h b/block/qcow2.h
index 99e4536..aae5f89 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -178,6 +178,7 @@ static inline int64_t align_offset(int64_t offset, int n)
 /* qcow2.c functions */
 int qcow2_backing_read1(BlockDriverState *bs, QEMUIOVector *qiov,
                   int64_t sector_num, int nb_sectors);
+int qcow2_update_header(BlockDriverState *bs);
 
 /* qcow2-refcount.c functions */
 int qcow2_refcount_init(BlockDriverState *bs);
-- 
1.7.6.5

[Qemu-devel] [PATCH 2/2] qcow2: Keep unknown header extension when rewriting header

[Kevin Wolf]

If we want header extensions to work as compatible extensions, we can't
destroy yet unknown header extensions when rewriting the header (e.g.
for changing the backing file). Save all unknown header extensions in a
list of blobs and include them in a new header.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c |   44 ++++++++++++++++++++++++++++++++++++++++++--
 block/qcow2.h |    8 ++++++++
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 9f8c2de..3692b45 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -77,8 +77,10 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
                                  uint64_t end_offset)
 {
+    BDRVQcowState *s = bs->opaque;
     QCowExtension ext;
     uint64_t offset;
+    int ret;
 
 #ifdef DEBUG_EXT
     printf("qcow2_read_extensions: start=%ld end=%ld\n", start_offset, end_offset);
@@ -129,8 +131,22 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             break;
 
         default:
-            /* unknown magic -- just skip it */
-            offset = ((offset + ext.len + 7) & ~7);
+            /* unknown magic - save it in case we need to rewrite the header */
+            {
+                Qcow2UnknownHeaderExtension *uext;
+
+                uext = g_malloc0(sizeof(*uext)  + ext.len);
+                uext->magic = ext.magic;
+                uext->len = ext.len;
+                QLIST_INSERT_HEAD(&s->unknown_header_ext, uext, next);
+
+                ret = bdrv_pread(bs->file, offset , uext->data, uext->len);
+                if (ret < 0) {
+                    return ret;
+                }
+
+                offset = ((offset + ext.len + 7) & ~7);
+            }
             break;
         }
     }
@@ -138,6 +154,16 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
     return 0;
 }
 
+static void cleanup_unknown_header_ext(BlockDriverState *bs)
+{
+    BDRVQcowState *s = bs->opaque;
+    Qcow2UnknownHeaderExtension *uext, *next;
+
+    QLIST_FOREACH_SAFE(uext, &s->unknown_header_ext, next, next) {
+        QLIST_REMOVE(uext, next);
+        g_free(uext);
+    }
+}
 
 static int qcow2_open(BlockDriverState *bs, int flags)
 {
@@ -291,6 +317,7 @@ static int qcow2_open(BlockDriverState *bs, int flags)
     return ret;
 
  fail:
+    cleanup_unknown_header_ext(bs);
     qcow2_free_snapshots(bs);
     qcow2_refcount_close(bs);
     g_free(s->l1_table);
@@ -632,6 +659,7 @@ static void qcow2_close(BlockDriverState *bs)
     qcow2_cache_destroy(bs, s->l2_table_cache);
     qcow2_cache_destroy(bs, s->refcount_block_cache);
 
+    cleanup_unknown_header_ext(bs);
     g_free(s->cluster_cache);
     qemu_vfree(s->cluster_data);
     qcow2_refcount_close(bs);
@@ -705,6 +733,7 @@ int qcow2_update_header(BlockDriverState *bs)
     int ret;
     uint64_t total_size;
     uint32_t refcount_table_clusters;
+    Qcow2UnknownHeaderExtension *uext;
 
     buf = qemu_blockalign(bs, buflen);
     memset(buf, 0, s->cluster_size);
@@ -752,6 +781,17 @@ int qcow2_update_header(BlockDriverState *bs)
         buflen -= ret;
     }
 
+    /* Keep unknown header extensions */
+    QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
+        ret = header_ext_add(buf, uext->magic, uext->data, uext->len, buflen);
+        if (ret < 0) {
+            goto fail;
+        }
+
+        buf += ret;
+        buflen -= ret;
+    }
+
     /* End of header extensions */
     ret = header_ext_add(buf, QCOW2_EXT_MAGIC_END, NULL, 0, buflen);
     if (ret < 0) {
diff --git a/block/qcow2.h b/block/qcow2.h
index aae5f89..fc35838 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -87,6 +87,13 @@ typedef struct QCowSnapshot {
 struct Qcow2Cache;
 typedef struct Qcow2Cache Qcow2Cache;
 
+typedef struct Qcow2UnknownHeaderExtension {
+    uint32_t magic;
+    uint32_t len;
+    QLIST_ENTRY(Qcow2UnknownHeaderExtension) next;
+    uint8_t data[];
+} Qcow2UnknownHeaderExtension;
+
 typedef struct BDRVQcowState {
     int cluster_bits;
     int cluster_size;
@@ -127,6 +134,7 @@ typedef struct BDRVQcowState {
     QCowSnapshot *snapshots;
 
     int flags;
+    QLIST_HEAD(, Qcow2UnknownHeaderExtension) unknown_header_ext;
 } BDRVQcowState;
 
 /* XXX: use std qcow open function ? */
-- 
1.7.6.5