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

[Anthony Liguori <aliguori@us.ibm.com>]

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.

> +    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?  
Can't a backing file name start immediately after the header?

Regards,

Anthony Liguori