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@xxxxxxxxxx> > Cc: Stefan Hajnoczi <stefan.hajnoczi@xxxxxxxxxx> > Cc: Anthony Liguori <aliguori@xxxxxxxxxxxxxxxxxx> > --- > 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@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list