[libvirt] [PATCH 4/4] Support for probing qed image metadata

[Eric Blake]

On 11/19/2010 09:18 AM, Adam Litke wrote:
> Implement getBackingStore() for QED images.  The header format is defined in
> the QED spec: http://wiki.qemu.org/Features/QED .
> 
> Signed-off-by: Adam Litke <agl at us.ibm.com>
> Cc: Stefan Hajnoczi <stefan.hajnoczi at uk.ibm.com>
> Cc: Anthony Liguori <aliguori at linux.vnet.ibm.com>
> ---
>  src/util/storage_file.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 77 insertions(+), 1 deletions(-)

Aha - I should have read this before commenting on patch 2.

> @@ -105,6 +107,11 @@ static int vmdk4GetBackingStore(char **, int *,
>  #define QCOW2_HDR_EXTENSION_BACKING_FORMAT 0xE2792ACA
>  
>  #define QED_HDR_IMAGE_SIZE 40
> +#define QED_HDR_FEATURES_OFFSET 16
> +#define QED_HDR_BACKING_FILE_OFFSET 56
> +#define QED_HDR_BACKING_FILE_SIZE 60
> +#define QED_F_BACKING_FILE 0x01
> +#define QED_F_BACKING_FORMAT_NO_PROBE 0x04

Again, I'll break the offsets into pieces.  See below.

>  
> +static unsigned long
> +qedGetHeaderUL(const unsigned char *loc)
> +{
> +    return ( ((unsigned long)loc[3] << 24)
> +           | ((unsigned long)loc[2] << 16)
> +           | ((unsigned long)loc[1] << 8)
> +           | ((unsigned long)loc[0] << 0));
> +}
> +
> +static unsigned long long
> +qedGetHeaderULL(const unsigned char *loc)
> +{
> +    return ( ((unsigned long)loc[7] << 56)
> +           | ((unsigned long)loc[6] << 48)
> +           | ((unsigned long)loc[5] << 40)
> +           | ((unsigned long)loc[4] << 32)
> +           | ((unsigned long)loc[3] << 24)
> +           | ((unsigned long)loc[2] << 16)
> +           | ((unsigned long)loc[1] << 8)
> +           | ((unsigned long)loc[0] << 0));
> +}

These two routines are independently useful for other little-endian
parsers in the same file.  Should we (as a separate patch) rename them
and put them to wider use, as well as adding big-endian counterparts for
the remaining file formats to share?  It would cut down on the number of
open-coded integer constructions elsewhere in the file.

> +
> +static int
> +qedGetBackingStore(char **res,
> +                   int *format,
> +                   const unsigned char *buf,
> +                   size_t buf_size)
> +{
> +    unsigned long long flags;
> +    unsigned long offset, size;
> +
> +    *res = NULL;
> +    /* Check if this image has a backing file */
> +    if (buf_size < QED_HDR_FEATURES_OFFSET+8)
> +        return BACKING_STORE_INVALID;
> +    flags = qedGetHeaderULL(buf + QED_HDR_FEATURES_OFFSET);
> +    if (!(flags & QED_F_BACKING_FILE))
> +        return BACKING_STORE_OK;
> +
> +    /* Parse the backing file */
> +    if (buf_size < QED_HDR_BACKING_FILE_OFFSET+8)
> +        return BACKING_STORE_INVALID;
> +    offset = qedGetHeaderUL(buf + QED_HDR_BACKING_FILE_OFFSET);
> +    if (offset > buf_size)
> +        return BACKING_STORE_INVALID;
> +    size = qedGetHeaderUL(buf + QED_HDR_BACKING_FILE_SIZE);
> +    if (size == 0)
> +        return BACKING_STORE_OK;
> +    if (offset + size > buf_size || offset + size < offset)
> +        return BACKING_STORE_INVALID;
> +    if (size + 1 == 0)
> +        return BACKING_STORE_INVALID;

This clause is redundant; you already rejected offset + size < offset,
and size + 1 == 0 implies size == -1, which would have failed that
earlier check.

ACK, with this squashed in.  I've pushed your series.

gdiff --git i/src/util/storage_file.c w/src/util/storage_file.c
index d7b4073..aa117e7 100644
--- i/src/util/storage_file.c
+++ w/src/util/storage_file.c
@@ -107,10 +107,10 @@ qedGetBackingStore(char **, int *, const unsigned
char *, size_t);
 #define QCOW2_HDR_EXTENSION_END 0
 #define QCOW2_HDR_EXTENSION_BACKING_FORMAT 0xE2792ACA

-#define QED_HDR_IMAGE_SIZE (4+4+4+4+8+8+8)
-#define QED_HDR_FEATURES_OFFSET 16
-#define QED_HDR_BACKING_FILE_OFFSET 56
-#define QED_HDR_BACKING_FILE_SIZE 60
+#define QED_HDR_FEATURES_OFFSET (4+4+4+4)
+#define QED_HDR_IMAGE_SIZE (QED_HDR_FEATURES_OFFSET+8+8+8)
+#define QED_HDR_BACKING_FILE_OFFSET (QED_HDR_IMAGE_SIZE+8+8)
+#define QED_HDR_BACKING_FILE_SIZE (QED_HDR_BACKING_FILE_OFFSET+4)
 #define QED_F_BACKING_FILE 0x01
 #define QED_F_BACKING_FORMAT_NO_PROBE 0x04

@@ -475,8 +475,6 @@ qedGetBackingStore(char **res,
         return BACKING_STORE_OK;
     if (offset + size > buf_size || offset + size < offset)
         return BACKING_STORE_INVALID;
-    if (size + 1 == 0)
-        return BACKING_STORE_INVALID;
     if (VIR_ALLOC_N(*res, size + 1) < 0) {
         virReportOOMError();
         return BACKING_STORE_ERROR;

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101119/0600a607/attachment-0001.sig>