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

[Uri Lublin <uril@redhat.com>]

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>
>> ---
>>  block-qcow2.c |  115 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  block.c       |   29 +++++++++++++-
>>  block.h       |    4 ++
>>  block_int.h   |    6 +++
>>  4 files changed, 149 insertions(+), 5 deletions(-)
>>
>> diff --git a/block-qcow2.c b/block-qcow2.c
>> index 8a5b621..d2263d5 100644
>> --- a/block-qcow2.c
>> +++ b/block-qcow2.c
>> @@ -45,6 +45,7 @@
>>  
>>  //#define DEBUG_ALLOC
>>  //#define DEBUG_ALLOC2
>> +//#define DEBUG_EXT
>>  
>>  #define QCOW_MAGIC (('Q' << 24) | ('F' << 16) | ('I' << 8) | 0xfb)
>>  #define QCOW_VERSION 2
>> @@ -77,6 +78,21 @@ typedef struct QCowHeader {
>>      uint64_t snapshots_offset;
>>  } QCowHeader;
>>  
>> +
>> +typedef struct {
>> +    /* +     *  Alternatives:
>> +     *    should I make them uint64_t ?
>> +     *    should I add version ?
>> +     *    should I use a single magic and add type field ?
>> +     *    should I keep number-of-extenstions ?
>> +     */
>>   
> 
> What you have is fine.  You should remove these comments though.

O.K. I'll remove those comments.

> 
>> +    uint32_t magic;
>> +    uint32_t len;
>> +} QCowExtension;
>> +#define  QCOW_EXT_MAGIC_END 0
>> +#define  QCOW_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>> +
>>  typedef struct __attribute__((packed)) QCowSnapshotHeader {
>>      /* header is 8 byte aligned */
>>      uint64_t l1_table_offset;
>> @@ -189,6 +205,66 @@ static int qcow_probe(const uint8_t *buf, int 
>> buf_size, const char *filename)
>>          return 0;
>>  }
>>  
>> +
>> +/* read qcow2 extension and fill bs
>> + * start reading from start_offset
>> + * finish reading upon magic of value 0 or when end_offset reached
>> + * unknown magic is skipped (future extension this version knows 
>> nothing about)
>> + * return 0 upon success, non-0 otherwise
>> + */
>>   
> 
> So, if this falls after the header but before the backing name, and you 
> need to find a '0, 0' combination to terminate the list, are we 
> guaranteed that all old qcow2 files are going to be parsed correctly?  

We are searching till one of the following conditions reached:
1. magic of value 0 or
2. end_offset reached (which is really the offset of the backing file name).

If you find it confusing, I can probably drop the first condition (which was 
added as a sanity check).
Also note that I'm ignoring unknown extensions (assumed to be written in the 
future), so magic of 0 will be skipped anyway.

> Can't a backing file name start immediately after the header?
It can. In that case the search will be over immediately,
since start_offset == end_offset (end-of-qcow2-header == 
offset-of-backing-file-name).

Thanks,
     Uri.