Re: [Qemu-devel] [PATCH 1/2] Introducing qcow2 extensions + keep backing file format (v4)

[Uri Lublin <uril@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2356 bytes --]

Anthony Liguori wrote:
> Uri Lublin wrote:
>> Qcow2 extensions are build of magic (id) len (in bytes) and data.
>> They reside between the end of the header and the filename.
>>
>> We can keep the backing file format in a such a qcow2 extension, 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 over host block devices.
>>    (only if the user specifically asks for it, by providing the format
>>    at creation time).
>>
>> I've added bdrv_create2 and drv->bdrv_create2 (implemented only
>> by block-qcow2 currently) to pass the backing-format to create.
>>
>> Based on a work done by Shahar Frank.
>>
>> Also fixes a security flaw found by Daniel P. Berrange on [1]
>> which summarizes: "Autoprobing: just say no."
>>
>> [1] http://lists.gnu.org/archive/html/qemu-devel/2008-12/msg01083.html
>>
>> Signed-off-by: Uri Lublin <uril@redhat.com>
>>   
> 
> This made it through my regression testing but...
> 
>>  int bdrv_create(BlockDriver *drv,
>>                  const char *filename, int64_t size_in_sectors,
>>                  const char *backing_file, int flags)
>> @@ -348,6 +362,9 @@ int bdrv_open2(BlockDriverState *bs, const char 
>> *filename, int flags,
>>  
>>          /* if there is a backing file, use it */
>>          bs1 = bdrv_new("");
>> +/*         if (drv) */
>> +/*             pstrcpy(bs1->backing_format, 
>> sizeof(bs->backing_format), */
>> +/*                     drv->format_name); */
>>   
> 
> I don't know if I missed this before or it was added since v3 but I'm 
> curious why this is here and why it's commented out.
> 

It's an old code, which should have been deleted.

The purpose was to make sure the temporary-snapshot-file backing-file-format
is known.
This is now done with bdrv_create2, that I've added.

You can delete it yourself or apply the attached patch (or squash patches)
Alternatively I can send it to you again as a single patch (v5)

Also I've noticed that a small fix is needed for BDRV_O_SNAPSHOT. e.g we should 
use bdrv_open2 (so we would not probe the image if the format is known), so I'm 
attaching a patch to fix that too. Again if you want I can resend it as a single 
patch (squashed with v4) or you can apply(squash) this patch yourself on top of 
my previous patches.

Thanks,
     Uri.

[-- Attachment #2: 0001-block.c-remove-dead-commented-out-code.patch --]
[-- Type: text/x-patch, Size: 801 bytes --]

>From 5003cc362793c10f276ab186026b3f25b6b3ac9e Mon Sep 17 00:00:00 2001
From: Uri Lublin <uril@redhat.com>
Date: Sun, 1 Mar 2009 12:43:45 +0200
Subject: [PATCH] block.c: remove dead commented out code

Signed-off-by: Uri Lublin <uril@redhat.com>
---
 block.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 31d6d5b..1626e0c 100644
--- a/block.c
+++ b/block.c
@@ -362,9 +362,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
 
         /* if there is a backing file, use it */
         bs1 = bdrv_new("");
-/*         if (drv) */
-/*             pstrcpy(bs1->backing_format, sizeof(bs->backing_format), */
-/*                     drv->format_name); */
         if (!bs1) {
             return -ENOMEM;
         }
-- 
1.6.0.6


[-- Attachment #3: 0002-block.c-If-image-format-is-known-use-it-for-BDRV_O.patch --]
[-- Type: text/x-patch, Size: 1238 bytes --]

>From bc79eae6c3226d21bd3fcbc370a86d7b24657aad Mon Sep 17 00:00:00 2001
From: Uri Lublin <uril@redhat.com>
Date: Sun, 1 Mar 2009 12:44:38 +0200
Subject: [PATCH] block.c: If image format is known, use it for BDRV_O_SNAPSHOT flag as well

Otherwise we'd
1. Unnecessarily probe the image to find out its format
2  Get it wrong for host-devices and non-raw images, which
   may affect created image size.

Also, force the temporary snapshot image to be of format bdrv_qcow2

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

diff --git a/block.c b/block.c
index 1626e0c..0939cad 100644
--- a/block.c
+++ b/block.c
@@ -365,7 +365,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
         if (!bs1) {
             return -ENOMEM;
         }
-        if (bdrv_open(bs1, filename, 0) < 0) {
+        if (bdrv_open2(bs1, filename, 0, drv) < 0) {
             bdrv_delete(bs1);
             return -1;
         }
@@ -392,6 +392,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
             return -1;
         }
         filename = tmp_filename;
+        drv = &bdrv_qcow2;
         bs->is_temporary = 1;
     }
 
-- 
1.6.0.6