[Qemu-devel] [PATCH 0/2] qemu block changes: keep backing file format v2

[Qemu-devel] [PATCH 0/2] qemu block changes: keep backing file format v2

[Uri Lublin]

The purpose of this patches is to keep the backing file format together
with its name, to
1. Provide a way to know the backing file format without probing
   it (setting the format at creation time).
2. Enable using qcow2 format (and others) over host block devices.
   (only if the user specifically asks for it, by providing the format).

I call "hidden image format" to the following format of a backing file name:
"name\0format". Although it can be considered a hack, that's an easy
way to support old images with new qemu as well as old qemu with new images
(in which case probing would be done), without changing the qcow2 header.

If a hidden format exists, use it for the backing file.
If no hidden format (or an unknown one) exists we go back to probing.

Based on a previous patch from Shahar Frank.
http://lists.gnu.org/archive/html/qemu-devel/2008-12/msg01083.html

The "name\0format" was suggested by Kevin Wolf on the above thread.

Also fixes a security flaw found by Daniel P. Berrange on that
same thread which summarizes: "Autoprobing: just say no."

The first patch introduces the hidden image format.
The second patch enable the user to specify the image format
for the backing file upon image creation.

Changes from v1: use "name\0format" instead of "fmt:FMT:name"

Examples:
### Let /dev/loop0 and /dev/loop1 be qemu-images of format qcow2 and raw.

### 1. Specify qcow2 backing file. demonstrates qcow2 over host block device
# ./qemu-img create -F qcow2 -b /dev/loop0 -f qcow2 /tmp/u0.qcow2
Formatting '/tmp/u0.qcow2', fmt=qcow2, backing_file=/dev/loop0, backing_fmt=qcow2, size=20971520 kB

# ./x86_64-softmmu/qemu-system-x86_64 -L pc-bios/ -drive file=/tmp/u0.qcow2,format=qcow2
Works with no probing.

### 2. Specify raw backing file.
# ./qemu-img create -F raw -b /dev/loop1 -f qcow2 /tmp/u1.qcow2
Formatting '/tmp/u1.qcow2', fmt=qcow2, backing_file=/dev/loop1, backing_fmt=raw, size=307200 kB

### 3. Do not specify backing file format (qcow2).
###    Probing of /dev/loop0 "finds" a "host_device" format (instead of qcow2)
###    and results with the guest fails to boot.
###    Note: wrong size upon create.
# ./qemu-img create -b /dev/loop0 -f qcow2 /tmp/p0.qcow2
Formatting '/tmp/p0.qcow2', fmt=qcow2, backing_file=/dev/loop0, size=4245292 kB

# ./x86_64-softmmu/qemu-system-x86_64 -L pc-bios/ -drive file=/tmp/p0.qcow2,format=qcow2
Probing /dev/loop0 and guest fails to boot.

### 4. Do not specify backing file format (raw).
###   Probing of /dev/loop1 "finds" a "host_device" format
###   and results with the guest successfully boot.
###   If however a VM with /dev/loop1 is was previously started and the guest
###   modified the beginning of the file (writing a fake qcow2 header)
###   there is a security problem as the guest user can now access almost 
###   any file on the host (e.g. /etc/passwd).
# ./qemu-img create -b /dev/loop1 -f qcow2 /tmp/p1.qcow2
Formatting '/tmp/p1.qcow2', fmt=qcow2, backing_file=/dev/loop1, size=307200 kB

# ./x86_64-softmmu/qemu-system-x86_64 -L pc-bios/ -drive file=/tmp/p1.qcow2,format=qcow2
Works with probing.

[Qemu-devel] [PATCH 1/2] Introducing hidden image format in backing file name.

[Uri Lublin]

The purpose of this patches is to keep the backing file format together
with its name, to
1. Provide a way to know the backing file format without probing
   it (setting the format at creation time).
2. Enable using qcow2 format (and others) over host block devices.
   (only if the user specifically asks for it, by providing the format).

I call "hidden image format" to the following format of a backing file name:
"name\0format". Although it can be considered a hack, that's an easy
way to support old images with new qemu as well as old qemu with new images
(in which case probing would be done), without changing the qcow2 header.

If a hidden format exists, use it for the backing file.
If no hidden format (or an unknown one) exists we go back to probing.

Based on a previous patch from Shahar Frank.
http://lists.gnu.org/archive/html/qemu-devel/2008-12/msg01083.html

The "name\0format" was suggested by Kevin Wolf on the above thread.

Also fixes a security flaw found by Daniel P. Berrange on that
same thread which summarizes: "Autoprobing: just say no."

Signed-off-by: Uri Lublin <uril@redhat.com>
---
 block-qcow2.c |    5 +++++
 block.c       |   27 ++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/block-qcow2.c b/block-qcow2.c
index 8a5b621..4b46b84 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -1499,6 +1499,11 @@ static int qcow_create(const char *filename, int64_t total_size,
     if (backing_file) {
         header.backing_file_offset = cpu_to_be64(header_size);
         backing_filename_len = strlen(backing_file);
+
+        /* account for hidden format */
+        if (backing_file[backing_filename_len + 1] != '\0')
+            backing_filename_len += strlen(backing_file +
+                                           backing_filename_len + 1) + 1;
         header.backing_file_size = cpu_to_be32(backing_filename_len);
         header_size += backing_filename_len;
     }
diff --git a/block.c b/block.c
index 50ec589..b43debc 100644
--- a/block.c
+++ b/block.c
@@ -254,6 +254,23 @@ static BlockDriver *find_protocol(const char *filename)
     return NULL;
 }
 
+/*
+ * if a hidden file format exist, returns it's appropriate drv
+ * otherwise returns NULL
+ */
+static BlockDriver* get_backing_file_format(const char *filename)
+{
+    BlockDriver *drv = NULL;
+    int len;
+
+    /* Support hidden format in filename: "name\0format\0" */
+    len = strlen(filename);
+    if (filename[len + 1] != '\0')
+        drv = bdrv_find_format(filename + len + 1);
+    return drv;
+}
+
+
 /* XXX: force raw format if block or character device ? It would
    simplify the BSD case */
 static BlockDriver *find_image_format(const char *filename)
@@ -263,6 +280,11 @@ static BlockDriver *find_image_format(const char *filename)
     uint8_t buf[2048];
     BlockDriverState *bs;
 
+    /* first check for hidden format */
+    drv = get_backing_file_format(filename);
+    if (drv)
+        return drv;
+
     /* detect host devices. By convention, /dev/cdrom[N] is always
        recognized as a host CDROM */
     if (strstart(filename, "/dev/cdrom", NULL))
@@ -423,6 +445,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     }
 #endif
     if (bs->backing_file[0] != '\0') {
+        BlockDriver *backing_drv ;
         /* if there is a backing file, use it */
         bs->backing_hd = bdrv_new("");
         if (!bs->backing_hd) {
@@ -430,9 +453,11 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
             bdrv_close(bs);
             return -ENOMEM;
         }
+        backing_drv = get_backing_file_format(bs->backing_file);
         path_combine(backing_filename, sizeof(backing_filename),
                      filename, bs->backing_file);
-        if (bdrv_open(bs->backing_hd, backing_filename, open_flags) < 0)
+        if (bdrv_open2(bs->backing_hd, backing_filename, open_flags,
+                       backing_drv) < 0)
             goto fail;
     }
 
-- 
1.6.0.6

[Qemu-devel] [PATCH 1/2] Introducing hidden image format in backing file name.

[Uri Lublin]

The purpose of this patch is to
1. Provide a way to know the backing file format without probing
   it (setting the format at creation time).
2. Enable using qcow2 format (and others) over host block devices.
   (only if the user specifically asks for it, by providing the format).

If no hidden format (or an unknown one) is provided we go back to probing.

I call "hidden image format" to the following format of a backing file name:
"name\0format". Although it can be considered a hack, that's an easy
way to support old images with new qemu as well as old qemu with new images
(in which case probing would be done), without changing the qcow2 header.

Based on a previous patch from Shahar Frank.
http://lists.gnu.org/archive/html/qemu-devel/2008-12/msg01083.html

The "name\0format" was suggested by Kevin Wolf on the above thread.

Also fixes a security flaw found by Daniel P. Berrange on that
same thread which summarizes: "Autoprobing: just say no."

Signed-off-by: Uri Lublin <uril@redhat.com>
---
 block-qcow2.c |    5 +++++
 block.c       |   27 ++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/block-qcow2.c b/block-qcow2.c
index 8a5b621..4b46b84 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -1499,6 +1499,11 @@ static int qcow_create(const char *filename, int64_t total_size,
     if (backing_file) {
         header.backing_file_offset = cpu_to_be64(header_size);
         backing_filename_len = strlen(backing_file);
+
+        /* account for hidden format */
+        if (backing_file[backing_filename_len + 1] != '\0')
+            backing_filename_len += strlen(backing_file +
+                                           backing_filename_len + 1) + 1;
         header.backing_file_size = cpu_to_be32(backing_filename_len);
         header_size += backing_filename_len;
     }
diff --git a/block.c b/block.c
index 50ec589..b43debc 100644
--- a/block.c
+++ b/block.c
@@ -254,6 +254,23 @@ static BlockDriver *find_protocol(const char *filename)
     return NULL;
 }
 
+/*
+ * if a hidden file format exist, returns it's appropriate drv
+ * otherwise returns NULL
+ */
+static BlockDriver* get_backing_file_format(const char *filename)
+{
+    BlockDriver *drv = NULL;
+    int len;
+
+    /* Support hidden format in filename: "name\0format\0" */
+    len = strlen(filename);
+    if (filename[len + 1] != '\0')
+        drv = bdrv_find_format(filename + len + 1);
+    return drv;
+}
+
+
 /* XXX: force raw format if block or character device ? It would
    simplify the BSD case */
 static BlockDriver *find_image_format(const char *filename)
@@ -263,6 +280,11 @@ static BlockDriver *find_image_format(const char *filename)
     uint8_t buf[2048];
     BlockDriverState *bs;
 
+    /* first check for hidden format */
+    drv = get_backing_file_format(filename);
+    if (drv)
+        return drv;
+
     /* detect host devices. By convention, /dev/cdrom[N] is always
        recognized as a host CDROM */
     if (strstart(filename, "/dev/cdrom", NULL))
@@ -423,6 +445,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     }
 #endif
     if (bs->backing_file[0] != '\0') {
+        BlockDriver *backing_drv ;
         /* if there is a backing file, use it */
         bs->backing_hd = bdrv_new("");
         if (!bs->backing_hd) {
@@ -430,9 +453,11 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
             bdrv_close(bs);
             return -ENOMEM;
         }
+        backing_drv = get_backing_file_format(bs->backing_file);
         path_combine(backing_filename, sizeof(backing_filename),
                      filename, bs->backing_file);
-        if (bdrv_open(bs->backing_hd, backing_filename, open_flags) < 0)
+        if (bdrv_open2(bs->backing_hd, backing_filename, open_flags,
+                       backing_drv) < 0)
             goto fail;
     }
 
-- 
1.6.0.6

[Qemu-devel] [PATCH 2/2] qemu-img: adding a "-F base_fmt" option to "qemu-img create -b"

[Uri Lublin]

If the user specifies the backing file format,
then when opening the backing file, there is no need
to probe the (backing file) image to figure out its format.

This follows my previous patch implementing hidden image format
within the backing file name.

Suggested by Daniel P. Berrange.

Signed-off-by: Uri Lublin <uril@redhat.com>
---
 qemu-img.c    |   33 ++++++++++++++++++++++++++++++---
 qemu-img.texi |    4 +++-
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6b852fb..29e5f45 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -25,6 +25,7 @@
 #include "osdep.h"
 #include "block_int.h"
 #include <assert.h>
+#include <stdio.h>
 
 #ifdef _WIN32
 #define WIN32_LEAN_AND_MEAN
@@ -58,7 +59,7 @@ static void help(void)
            "QEMU disk image utility\n"
            "\n"
            "Command syntax:\n"
-           "  create [-e] [-6] [-b base_image] [-f fmt] filename [size]\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"
            "  info [-f fmt] filename\n"
@@ -218,21 +219,26 @@ static int img_create(int argc, char **argv)
 {
     int c, ret, flags;
     const char *fmt = "raw";
+    const char *base_fmt = NULL;
     const char *filename;
     const char *base_filename = NULL;
     uint64_t size;
     const char *p;
     BlockDriver *drv;
+    char filename_buff[1024];
 
     flags = 0;
     for(;;) {
-        c = getopt(argc, argv, "b:f:he6");
+        c = getopt(argc, argv, "F:b:f:he6");
         if (c == -1)
             break;
         switch(c) {
         case 'h':
             help();
             break;
+        case 'F':
+            base_fmt = optarg;
+            break;
         case 'b':
             base_filename = optarg;
             break;
@@ -253,7 +259,15 @@ static int img_create(int argc, char **argv)
     size = 0;
     if (base_filename) {
         BlockDriverState *bs;
-        bs = bdrv_new_open(base_filename, NULL);
+        BlockDriver *base_drv = NULL;
+        
+        if (base_fmt) {
+            base_drv = bdrv_find_format(base_fmt);
+            if (base_drv == NULL)
+                error("Unknown basefile format '%s'", base_fmt);
+        }
+        
+        bs = bdrv_new_open(base_filename, base_fmt);
         bdrv_get_geometry(bs, &size);
         size *= 512;
         bdrv_delete(bs);
@@ -284,8 +298,21 @@ static int img_create(int argc, char **argv)
     if (base_filename) {
         printf(", backing_file=%s",
                base_filename);
+        if (base_fmt)
+            printf(", backing_fmt=%s",
+                   base_fmt);
     }
     printf(", size=%" PRIu64 " kB\n", size / 1024);
+    if (base_filename) {
+        memset(filename_buff, 0, sizeof(filename_buff));
+        pstrcpy(filename_buff, sizeof(filename_buff), base_filename);
+        if (base_fmt) {
+            int len;
+            len = strlen(filename_buff);
+            pstrcat(filename_buff + len + 1, sizeof(filename_buff) - (len + 1), base_fmt);
+        }
+        base_filename = filename_buff;
+    }
     ret = bdrv_create(drv, filename, size / 512, base_filename, flags);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
diff --git a/qemu-img.texi b/qemu-img.texi
index a40f841..deef2ab 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -8,7 +8,7 @@ usage: qemu-img command [command options]
 
 The following commands are supported:
 @table @option
-@item create [-e] [-6] [-b @var{base_image}] [-f @var{fmt}] @var{filename} [@var{size}]
+@item create [-e] [-6] [-F @var{base_fmt}] [-b @var{base_image}] [-f @var{fmt}] @var{filename} [@var{size}]
 @item commit [-f @var{fmt}] @var{filename}
 @item convert [-c] [-e] [-6] [-f @var{fmt}] [-O @var{output_fmt}] [-B @var{output_base_image}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 @item info [-f @var{fmt}] @var{filename}
@@ -27,6 +27,8 @@ forces the output image to be created as a copy on write
 image of the specified base image; @code{output_base_image} should have the same
 content as the input's base image, however the path, image format, etc may
 differ
+@item base_fmt
+is the disk image format of @var{base_image}. for more information look at @var{fmt}
 @item fmt
 is the disk image format. It is guessed automatically in most cases. The following formats are supported:
 
-- 
1.6.0.6

Re: [Qemu-devel] [PATCH 0/2] qemu block changes: keep backing file format v2

[Jamie Lokier]

Uri Lublin wrote:
> I call "hidden image format" to the following format of a backing file name:
> "name\0format". Although it can be considered a hack, that's an easy
> way to support old images with new qemu as well as old qemu with new images
> (in which case probing would be done), without changing the qcow2 header.
...
> Changes from v1: use "name\0format" instead of "fmt:FMT:name"

Do you think there will be any other information added in future,
while wanting to remain compatible with old qemus (i.e. without
changing the qcow2 header)?

If so, "name\0format=FMT", and stop at the second \0 if there is one,
something like that?

-- Jamie

Re: [Qemu-devel] [PATCH 0/2] qemu block changes: keep backing file format v2

[Uri Lublin]

Jamie Lokier wrote:
> Uri Lublin wrote:
>> I call "hidden image format" to the following format of a backing file name:
>> "name\0format". Although it can be considered a hack, that's an easy
>> way to support old images with new qemu as well as old qemu with new images
>> (in which case probing would be done), without changing the qcow2 header.
> ...
>> Changes from v1: use "name\0format" instead of "fmt:FMT:name"
> 
> Do you think there will be any other information added in future,
> while wanting to remain compatible with old qemus (i.e. without
> changing the qcow2 header)?
> 
> If so, "name\0format=FMT", and stop at the second \0 if there is one,
> something like that?
> 
> -- Jamie

I think you'd always want to keep the format of your backing file, so as long as
you keep the order of options fixed, it's safe (meaning we can add more options 
following the format, and there is no need to parse it).

If we are thinking about ways to extend the simple "name\0options" format,
Shahar Frank wrote a better, more extensible implementation, which saves image 
options as extensions following the qcow2-header. The options are saved in a 
binary format similar to the header. Each option has a little header of its own 
which includes magic(==type) and len.

We (Shahar) also implemented a zero-cluster (aka zero-dedup) and would like,
in the future, to keep the offset of that cluster.

If such an approach is acceptable, I can work on a patch implementing it (rebase 
and improve a bit Shahar's implementation).

Thanks,
     Uri.

Re: [Qemu-devel] [PATCH 0/2] qemu block changes: keep backing file format v2

[Anthony Liguori]

Uri Lublin wrote:
> Jamie Lokier wrote:
>> Uri Lublin wrote:
>>> I call "hidden image format" to the following format of a backing 
>>> file name:
>>> "name\0format". Although it can be considered a hack, that's an easy
>>> way to support old images with new qemu as well as old qemu with new 
>>> images
>>> (in which case probing would be done), without changing the qcow2 
>>> header.
>> ...
>>> Changes from v1: use "name\0format" instead of "fmt:FMT:name"
>>
>> Do you think there will be any other information added in future,
>> while wanting to remain compatible with old qemus (i.e. without
>> changing the qcow2 header)?
>>
>> If so, "name\0format=FMT", and stop at the second \0 if there is one,
>> something like that?
>>
>> -- Jamie
>
> I think you'd always want to keep the format of your backing file, so 
> as long as
> you keep the order of options fixed, it's safe (meaning we can add 
> more options following the format, and there is no need to parse it).
>
> If we are thinking about ways to extend the simple "name\0options" 
> format,

How about just extending the qcow2 header instead of playing ugly games 
with the backing_file field?

Regards,

Anthony Liguori

> Shahar Frank wrote a better, more extensible implementation, which 
> saves image options as extensions following the qcow2-header. The 
> options are saved in a binary format similar to the header. Each 
> option has a little header of its own which includes magic(==type) and 
> len.
>
> We (Shahar) also implemented a zero-cluster (aka zero-dedup) and would 
> like,
> in the future, to keep the offset of that cluster.
>
> If such an approach is acceptable, I can work on a patch implementing 
> it (rebase and improve a bit Shahar's implementation).
>
> Thanks,
>     Uri.
>
>
>

Re: [Qemu-devel] [PATCH 0/2] qemu block changes: keep backing file format v2

[Uri Lublin]

Anthony Liguori wrote:
> Uri Lublin wrote:
>> Jamie Lokier wrote:
>>> Uri Lublin wrote:
>>>> I call "hidden image format" to the following format of a backing 
>>>> file name:
>>>> "name\0format". Although it can be considered a hack, that's an easy
>>>> way to support old images with new qemu as well as old qemu with new 
>>>> images
>>>> (in which case probing would be done), without changing the qcow2 
>>>> header.
>>> ...
>>>> Changes from v1: use "name\0format" instead of "fmt:FMT:name"
>>>
>>> Do you think there will be any other information added in future,
>>> while wanting to remain compatible with old qemus (i.e. without
>>> changing the qcow2 header)?
>>>
>>> If so, "name\0format=FMT", and stop at the second \0 if there is one,
>>> something like that?
>>>
>>> -- Jamie
>>
>> I think you'd always want to keep the format of your backing file, so 
>> as long as
>> you keep the order of options fixed, it's safe (meaning we can add 
>> more options following the format, and there is no need to parse it).
>>
>> If we are thinking about ways to extend the simple "name\0options" 
>> format,
> 
> How about just extending the qcow2 header instead of playing ugly games 
> with the backing_file field?
> 

Would that be considered qcow3 ?

Shahar's extensions are following in the qcow2 header (without breaking existing 
images). I forgot to mention they are only implemented for qcow2 images.

Currently backing files are being opened in block.c:bdrv_open2(). If we want to 
do something specific to qcow2 (with regards to the backing file format), we'll 
need to leave bs->backing_file empty and open the backing file from 
block-qcow2.c:qcow_open(). Or we can add a backing_format field to struct 
BlockDriverState.

Regards,
     Uri.

>> Shahar Frank wrote a better, more extensible implementation, which 
>> saves image options as extensions following the qcow2-header. The 
>> options are saved in a binary format similar to the header. Each 
>> option has a little header of its own which includes magic(==type) and 
>> len.
>>
>> We (Shahar) also implemented a zero-cluster (aka zero-dedup) and would 
>> like,
>> in the future, to keep the offset of that cluster.
>>
>> If such an approach is acceptable, I can work on a patch implementing 
>> it (rebase and improve a bit Shahar's implementation).
>>
>> Thanks,
>>     Uri.