Skip to contentSkip to conversation

qcow2: Fix header update with overridden backing file

PATCH Applied Kevin Wolf 3 participants 3 messages Apr 7, 2015 – Apr 8, 2015 Block layer

lore
Share & tools
lore

Kevin Wolf

author
#0 lore raw

PATCH v2 1/1 qcow2: Fix header update with overridden backing file

In recent qemu versions, it is possible to override the backing file name and format that is stored in the image file with values given at runtime. In such cases, the temporary override could end up in the image header if the qcow2 header was updated, while obviously correct behaviour would be to leave the on-disk backing file path/format unchanged.

Fix this and add a test case for it.

---
v2:
- Now freeing instead of leaking the new strings in qcow2_close()

 block/qcow2.c              | 29 ++++++++++----
 block/qcow2.h              |  6 +++
 tests/qemu-iotests/130     | 95 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/130.out | 43 +++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 5 files changed, 167 insertions(+), 7 deletions(-)
 create mode 100755 tests/qemu-iotests/130
 create mode 100644 tests/qemu-iotests/130.out
block/qcow2.c +22 -7
index 32bdf75..316a8db 100644
@@ -140,6 +140,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
                 return 3;
             }
             bs->backing_format[ext.len] = '\0';
+            s->image_backing_format = g_strdup(bs->backing_format);
 #ifdef DEBUG_EXT
             printf("Qcow2: Got format extension %s\n", bs->backing_format);
 #endif
@@ -884,6 +885,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
             goto fail;
         }
         bs->backing_file[len] = '\0';
+        s->image_backing_file = g_strdup(bs->backing_file);
     }
 
     /* Internal snapshots */
@@ -1457,6 +1459,9 @@ static void qcow2_close(BlockDriverState *bs)
     g_free(s->unknown_header_fields);
     cleanup_unknown_header_ext(bs);
 
+    g_free(s->image_backing_file);
+    g_free(s->image_backing_format);
+
     g_free(s->cluster_cache);
     qemu_vfree(s->cluster_data);
     qcow2_refcount_close(bs);
@@ -1622,9 +1627,10 @@ int qcow2_update_header(BlockDriverState *bs)
     }
 
     /* Backing file format header extension */
-    if (*bs->backing_format) {
+    if (s->image_backing_format) {
         ret = header_ext_add(buf, QCOW2_EXT_MAGIC_BACKING_FORMAT,
-                             bs->backing_format, strlen(bs->backing_format),
+                             s->image_backing_format,
+                             strlen(s->image_backing_format),
                              buflen);
         if (ret < 0) {
             goto fail;
@@ -1682,8 +1688,8 @@ int qcow2_update_header(BlockDriverState *bs)
     buflen -= ret;
 
     /* Backing file name */
-    if (*bs->backing_file) {
-        size_t backing_file_len = strlen(bs->backing_file);
+    if (s->image_backing_file) {
+        size_t backing_file_len = strlen(s->image_backing_file);
 
         if (buflen < backing_file_len) {
             ret = -ENOSPC;
@@ -1691,7 +1697,7 @@ int qcow2_update_header(BlockDriverState *bs)
         }
 
         /* Using strncpy is ok here, since buf is not NUL-terminated. */
-        strncpy(buf, bs->backing_file, buflen);
+        strncpy(buf, s->image_backing_file, buflen);
 
         header->backing_file_offset = cpu_to_be64(buf - ((char*) header));
         header->backing_file_size   = cpu_to_be32(backing_file_len);
@@ -1712,9 +1718,17 @@ fail:
 static int qcow2_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt)
 {
+    BDRVQcowState *s = bs->opaque;
+
     pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_file ?: "");
     pstrcpy(bs->backing_format, sizeof(bs->backing_format), backing_fmt ?: "");
 
+    g_free(s->image_backing_file);
+    g_free(s->image_backing_format);
+
+    s->image_backing_file = backing_file ? g_strdup(bs->backing_file) : NULL;
+    s->image_backing_format = backing_fmt ? g_strdup(bs->backing_format) : NULL;
+
     return qcow2_update_header(bs);
 }
 
@@ -2751,8 +2765,9 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
     }
 
     if (backing_file || backing_format) {
-        ret = qcow2_change_backing_file(bs, backing_file ?: bs->backing_file,
-                                        backing_format ?: bs->backing_format);
+        ret = qcow2_change_backing_file(bs,
+                    backing_file ?: s->image_backing_file,
+                    backing_format ?: s->image_backing_format);
         if (ret < 0) {
             return ret;
         }
block/qcow2.h +6 -0
index aa6d367..422b825 100644
@@ -283,6 +283,12 @@ typedef struct BDRVQcowState {
     QLIST_HEAD(, Qcow2UnknownHeaderExtension) unknown_header_ext;
     QTAILQ_HEAD (, Qcow2DiscardRegion) discards;
     bool cache_discards;
+
+    /* Backing file path and format as stored in the image (this is not the
+     * effective path/format, which may be the result of a runtime option
+     * override) */
+    char *image_backing_file;
+    char *image_backing_format;
 } BDRVQcowState;
 
 struct QCowAIOCB;
tests/qemu-iotests/130 new file +95 -0

95 lines not on this page · raw message

tests/qemu-iotests/130.out new file +43 -0

43 lines not on this page · raw message

tests/qemu-iotests/group +1 -0
index 6262127..bcf2578 100644
@@ -124,3 +124,4 @@
 121 rw auto
 123 rw auto quick
 128 rw auto quick
+130 rw auto quick
Signature · 1 line
1.8.3.1
Reviewed-by on v2 1/1
#1 lore raw
Quoted 15 lines · Kevin Wolf

In recent qemu versions, it is possible to override the backing file name and format that is stored in the image file with values given at runtime. In such cases, the temporary override could end up in the image header if the qcow2 header was updated, while obviously correct behaviour would be to leave the on-disk backing file path/format unchanged.

Fix this and add a test case for it.

---
v2:
- Now freeing instead of leaking the new strings in qcow2_close()

Quoted 8 lines
 block/qcow2.c              | 29 ++++++++++----
 block/qcow2.h              |  6 +++
 tests/qemu-iotests/130     | 95 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/130.out | 43 +++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 5 files changed, 167 insertions(+), 7 deletions(-)
 create mode 100755 tests/qemu-iotests/130
 create mode 100644 tests/qemu-iotests/130.out
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#2 lore raw
In reply to Kevin Wolf
Quoted 15 lines · Kevin Wolf

In recent qemu versions, it is possible to override the backing file name and format that is stored in the image file with values given at runtime. In such cases, the temporary override could end up in the image header if the qcow2 header was updated, while obviously correct behaviour would be to leave the on-disk backing file path/format unchanged.

Fix this and add a test case for it.

---
v2:
- Now freeing instead of leaking the new strings in qcow2_close()

Quoted 8 lines
 block/qcow2.c              | 29 ++++++++++----
 block/qcow2.h              |  6 +++
 tests/qemu-iotests/130     | 95 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/130.out | 43 +++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 5 files changed, 167 insertions(+), 7 deletions(-)
 create mode 100755 tests/qemu-iotests/130
 create mode 100644 tests/qemu-iotests/130.out

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan