[libvirt] [PATCH 0/4] Support the QED disk image format (V2)
Changes since V1: - Fix virStorageFileMatchesVersion() for formats without version info - Allow backingStore format probing for QED images since it is safe Qemu is about to gain support for a new disk image format: QED. Details on this format (including specification) can be found here: http://wiki.qemu.org/Features/QED. This short series of patches allows QED images to be used with libvirt. Adam Litke (4): Allow probing of image formats without version information QED: Basic support for QED images storage_file: Add a new flag to mark backing files that are safe to probe Support for probing qed image metadata src/conf/domain_conf.c | 4 ++ src/util/storage_file.c | 89 +++++++++++++++++++++++++++++++++++++++++++++-- src/util/storage_file.h | 2 + 3 files changed, 92 insertions(+), 3 deletions(-) -- 1.7.3.2.164.g6f10c
Disk image formats that wish to opt-out of version validation are supposed to set versionOffset to -1 in their fileTypeInfo entry. By unconditionally returning False for these formats, virStorageFileMatchesVersion() incorrectly reports a version mismatch when the test was actually skipped. The correct behavior is to return True so these formats can be successfully probed using the magic bytes alone. Signed-off-by: Adam Litke <agl@us.ibm.com> --- src/util/storage_file.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 4098383..f8ab168 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -478,7 +478,7 @@ virStorageFileMatchesVersion(int format, /* Validate version number info */ if (fileTypeInfo[format].versionOffset == -1) - return false; + return true; if ((fileTypeInfo[format].versionOffset + 4) > buflen) return false; -- 1.7.3.2.164.g6f10c
On 11/19/2010 09:18 AM, Adam Litke wrote:
Disk image formats that wish to opt-out of version validation are supposed to set versionOffset to -1 in their fileTypeInfo entry.
By unconditionally returning False for these formats, virStorageFileMatchesVersion() incorrectly reports a version mismatch when the test was actually skipped. The correct behavior is to return True so these formats can be successfully probed using the magic bytes alone.
Signed-off-by: Adam Litke <agl@us.ibm.com> --- src/util/storage_file.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 4098383..f8ab168 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -478,7 +478,7 @@ virStorageFileMatchesVersion(int format,
/* Validate version number info */ if (fileTypeInfo[format].versionOffset == -1) - return false; + return true;
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Add an entry in fileTypeInfo for QED image files. Signed-off-by: Adam Litke <agl@us.ibm.com> Cc: Stefan Hajnoczi <stefan.hajnoczi@uk.ibm.com> Cc: Anthony Liguori <aliguori@linux.vnet.ibm.com> --- src/util/storage_file.c | 9 ++++++++- src/util/storage_file.h | 1 + 2 files changed, 9 insertions(+), 1 deletions(-) diff --git a/src/util/storage_file.c b/src/util/storage_file.c index f8ab168..27aad26 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -43,7 +43,7 @@ VIR_ENUM_IMPL(virStorageFileFormat, VIR_STORAGE_FILE_LAST, "raw", "dir", "bochs", "cloop", "cow", "dmg", "iso", - "qcow", "qcow2", "vmdk", "vpc") + "qcow", "qcow2", "qed", "vmdk", "vpc") enum lv_endian { LV_LITTLE_ENDIAN = 1, /* 1234 */ @@ -104,6 +104,8 @@ static int vmdk4GetBackingStore(char **, int *, #define QCOW2_HDR_EXTENSION_END 0 #define QCOW2_HDR_EXTENSION_BACKING_FORMAT 0xE2792ACA +#define QED_HDR_IMAGE_SIZE 40 + /* VMDK needs at least this to find backing store, * other formats need less */ #define STORAGE_MAX_HEAD (20*512) @@ -151,6 +153,11 @@ static struct FileTypeInfo const fileTypeInfo[] = { LV_BIG_ENDIAN, 4, 2, QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW2_HDR_CRYPT, qcow2GetBackingStore, }, + [VIR_STORAGE_FILE_QED] = { + "QED\0", NULL, + LV_LITTLE_ENDIAN, -1, -1, + QED_HDR_IMAGE_SIZE, 8, 1, -1, NULL, + }, [VIR_STORAGE_FILE_VMDK] = { "KDMV", NULL, LV_LITTLE_ENDIAN, 4, 1, diff --git a/src/util/storage_file.h b/src/util/storage_file.h index a3703f5..c4d4650 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -38,6 +38,7 @@ enum virStorageFileFormat { VIR_STORAGE_FILE_ISO, VIR_STORAGE_FILE_QCOW, VIR_STORAGE_FILE_QCOW2, + VIR_STORAGE_FILE_QED, VIR_STORAGE_FILE_VMDK, VIR_STORAGE_FILE_VPC, VIR_STORAGE_FILE_LAST, -- 1.7.3.2.164.g6f10c
On 11/19/2010 09:18 AM, Adam Litke wrote:
Add an entry in fileTypeInfo for QED image files.
Signed-off-by: Adam Litke <agl@us.ibm.com> Cc: Stefan Hajnoczi <stefan.hajnoczi@uk.ibm.com> Cc: Anthony Liguori <aliguori@linux.vnet.ibm.com> --- src/util/storage_file.c | 9 ++++++++- src/util/storage_file.h | 1 + 2 files changed, 9 insertions(+), 1 deletions(-)
+#define QED_HDR_IMAGE_SIZE 40
Given that other formats broke it out by fields, I did likewise for this one.
+ /* VMDK needs at least this to find backing store, * other formats need less */ #define STORAGE_MAX_HEAD (20*512) @@ -151,6 +153,11 @@ static struct FileTypeInfo const fileTypeInfo[] = { LV_BIG_ENDIAN, 4, 2, QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW2_HDR_CRYPT, qcow2GetBackingStore, }, + [VIR_STORAGE_FILE_QED] = {
I'm amending this patch to add a comment to a decent reference URL (other file types should probably likewise add a reference URL, but that can come later).
+ "QED\0", NULL, + LV_LITTLE_ENDIAN, -1, -1, + QED_HDR_IMAGE_SIZE, 8, 1, -1, NULL,
Why no backing store extraction function? Not a show-stopper to this patch, but something to consider adding. ACK as amended: diff --git i/src/util/storage_file.c w/src/util/storage_file.c index 5fe11a3..c011544 100644 --- i/src/util/storage_file.c +++ w/src/util/storage_file.c @@ -105,7 +105,7 @@ static int vmdk4GetBackingStore(char **, int *, #define QCOW2_HDR_EXTENSION_END 0 #define QCOW2_HDR_EXTENSION_BACKING_FORMAT 0xE2792ACA -#define QED_HDR_IMAGE_SIZE 40 +#define QED_HDR_IMAGE_SIZE (4+4+4+4+8+8+8) /* VMDK needs at least this to find backing store, * other formats need less */ @@ -155,6 +155,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW2_HDR_CRYPT, qcow2GetBackingStore, }, [VIR_STORAGE_FILE_QED] = { + /* http://wiki.qemu.org/Features/QED */ "QED\0", NULL, LV_LITTLE_ENDIAN, -1, -1, QED_HDR_IMAGE_SIZE, 8, 1, -1, NULL, -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, 2010-11-19 at 16:05 -0700, Eric Blake wrote:
Why no backing store extraction function? Not a show-stopper to this patch, but something to consider adding.
Thanks for the review Eric. Please see patch 4 where I have implemented qedGetBackingStore(). -- Thanks, Adam
Signed-off-by: Adam Litke <agl@us.ibm.com> --- src/conf/domain_conf.c | 4 ++++ src/util/storage_file.c | 2 +- src/util/storage_file.h | 1 + 3 files changed, 6 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2d11785..a08c846 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7825,6 +7825,10 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, if (format == VIR_STORAGE_FILE_AUTO && !allowProbing) format = VIR_STORAGE_FILE_RAW; /* Stops further recursion */ + + /* Allow probing for image formats that are safe */ + if (format == VIR_STORAGE_FILE_AUTO_SAFE) + format = VIR_STORAGE_FILE_AUTO; } while (nextpath); ret = 0; diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 27aad26..b656557 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -41,7 +41,7 @@ VIR_ENUM_IMPL(virStorageFileFormat, VIR_STORAGE_FILE_LAST, - "raw", "dir", "bochs", + "raw", "probe", "dir", "bochs", "cloop", "cow", "dmg", "iso", "qcow", "qcow2", "qed", "vmdk", "vpc") diff --git a/src/util/storage_file.h b/src/util/storage_file.h index c4d4650..13c731f 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -30,6 +30,7 @@ enum virStorageFileFormat { VIR_STORAGE_FILE_AUTO = -1, VIR_STORAGE_FILE_RAW = 0, + VIR_STORAGE_FILE_AUTO_SAFE, VIR_STORAGE_FILE_DIR, VIR_STORAGE_FILE_BOCHS, VIR_STORAGE_FILE_CLOOP, -- 1.7.3.2.164.g6f10c
On 11/19/2010 09:18 AM, Adam Litke wrote:
Signed-off-by: Adam Litke <agl@us.ibm.com> --- src/conf/domain_conf.c | 4 ++++ src/util/storage_file.c | 2 +- src/util/storage_file.h | 1 + 3 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2d11785..a08c846 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7825,6 +7825,10 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, if (format == VIR_STORAGE_FILE_AUTO && !allowProbing) format = VIR_STORAGE_FILE_RAW; /* Stops further recursion */ + + /* Allow probing for image formats that are safe */ + if (format == VIR_STORAGE_FILE_AUTO_SAFE) + format = VIR_STORAGE_FILE_AUTO; } while (nextpath);
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Nov 19, 2010 at 10:18:17AM -0600, Adam Litke wrote:
Signed-off-by: Adam Litke <agl@us.ibm.com> --- src/conf/domain_conf.c | 4 ++++ src/util/storage_file.c | 2 +- src/util/storage_file.h | 1 + 3 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2d11785..a08c846 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7825,6 +7825,10 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, if (format == VIR_STORAGE_FILE_AUTO && !allowProbing) format = VIR_STORAGE_FILE_RAW; /* Stops further recursion */ + + /* Allow probing for image formats that are safe */ + if (format == VIR_STORAGE_FILE_AUTO_SAFE) + format = VIR_STORAGE_FILE_AUTO; } while (nextpath);
ret = 0; diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 27aad26..b656557 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -41,7 +41,7 @@
VIR_ENUM_IMPL(virStorageFileFormat, VIR_STORAGE_FILE_LAST, - "raw", "dir", "bochs", + "raw", "probe", "dir", "bochs", "cloop", "cow", "dmg", "iso", "qcow", "qcow2", "qed", "vmdk", "vpc")
diff --git a/src/util/storage_file.h b/src/util/storage_file.h index c4d4650..13c731f 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -30,6 +30,7 @@ enum virStorageFileFormat { VIR_STORAGE_FILE_AUTO = -1, VIR_STORAGE_FILE_RAW = 0, + VIR_STORAGE_FILE_AUTO_SAFE,
You need to make sure that this is set to '-2', otherwise this value becomes parsable in the XML which is not desired. THis avoids the need to add it to the enum above.
VIR_STORAGE_FILE_DIR, VIR_STORAGE_FILE_BOCHS, VIR_STORAGE_FILE_CLOOP,
Daniel
On Mon, 2010-11-22 at 11:04 +0000, Daniel P. Berrange wrote:
diff --git a/src/util/storage_file.h b/src/util/storage_file.h index c4d4650..13c731f 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -30,6 +30,7 @@ enum virStorageFileFormat { VIR_STORAGE_FILE_AUTO = -1, VIR_STORAGE_FILE_RAW = 0, + VIR_STORAGE_FILE_AUTO_SAFE,
You need to make sure that this is set to '-2', otherwise this value becomes parsable in the XML which is not desired. THis avoids the need to add it to the enum above.
Ah, of course. I'll respin the series to incorporate this, and some of the other items Eric suggested.
VIR_STORAGE_FILE_DIR, VIR_STORAGE_FILE_BOCHS, VIR_STORAGE_FILE_CLOOP,
Daniel
-- Thanks, Adam
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@us.ibm.com> Cc: Stefan Hajnoczi <stefan.hajnoczi@uk.ibm.com> Cc: Anthony Liguori <aliguori@linux.vnet.ibm.com> --- src/util/storage_file.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 77 insertions(+), 1 deletions(-) diff --git a/src/util/storage_file.c b/src/util/storage_file.c index b656557..b195ef7 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -89,6 +89,8 @@ static int qcow2GetBackingStore(char **, int *, const unsigned char *, size_t); static int vmdk4GetBackingStore(char **, int *, const unsigned char *, size_t); +static int +qedGetBackingStore(char **, int *, const unsigned char *, size_t); #define QCOWX_HDR_VERSION (4) #define QCOWX_HDR_BACKING_FILE_OFFSET (QCOWX_HDR_VERSION+4) @@ -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 /* VMDK needs at least this to find backing store, * other formats need less */ @@ -156,7 +163,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { [VIR_STORAGE_FILE_QED] = { "QED\0", NULL, LV_LITTLE_ENDIAN, -1, -1, - QED_HDR_IMAGE_SIZE, 8, 1, -1, NULL, + QED_HDR_IMAGE_SIZE, 8, 1, -1, qedGetBackingStore, }, [VIR_STORAGE_FILE_VMDK] = { "KDMV", NULL, @@ -416,6 +423,75 @@ cleanup: return ret; } +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)); +} + +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; + if (VIR_ALLOC_N(*res, size + 1) < 0) { + virReportOOMError(); + return BACKING_STORE_ERROR; + } + memcpy(*res, buf + offset, size); + (*res)[size] = '\0'; + + if (format) { + if (flags & QED_F_BACKING_FORMAT_NO_PROBE) + *format = virStorageFileFormatTypeFromString("raw"); + else + *format = VIR_STORAGE_FILE_AUTO_SAFE; + } + + return BACKING_STORE_OK; +} + /** * Return an absolute path corresponding to PATH, which is absolute or relative * to the directory containing BASE_FILE, or NULL on error -- 1.7.3.2.164.g6f10c
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@us.ibm.com> Cc: Stefan Hajnoczi <stefan.hajnoczi@uk.ibm.com> Cc: Anthony Liguori <aliguori@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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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 .
+ if (offset + size > buf_size || offset + size < offset) + return BACKING_STORE_INVALID;
As currently coded, buf_size is at most STORAGE_MAX_HEAD (20*512). QED does not appear to have any maximum header size (other than the fact that header size is a multiple of cluster size), and permits a cluster size of 2**26. I don't see anything on the QED file format that requires the backing_filename to occur within the header clusters (that is, shouldn't QED add a file format restriction that backing_filename_offset+backing_filename_size must be less than the start of the first regular cluster?). More worrying, I don't see anything in QED that requires the filename to occur within the first 10K bytes. Do we need to add another enum value to libvirt's backing store callback routine, to be used when the header requests data that lies beyond buf_size but is still feasibly valid, for the case where QED designates a backing store location that is beyond 10k but still before the start of the first cluster, rather than the current approach of just treating it as BACKING_STORE_INVALID? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Nov 19, 2010 at 11:54 PM, Eric Blake <eblake@redhat.com> wrote:
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 .
+ if (offset + size > buf_size || offset + size < offset) + return BACKING_STORE_INVALID;
As currently coded, buf_size is at most STORAGE_MAX_HEAD (20*512). QED does not appear to have any maximum header size (other than the fact that header size is a multiple of cluster size), and permits a cluster size of 2**26.
I don't see anything on the QED file format that requires the backing_filename to occur within the header clusters (that is, shouldn't QED add a file format restriction that backing_filename_offset+backing_filename_size must be less than the start of the first regular cluster?).
More worrying, I don't see anything in QED that requires the filename to occur within the first 10K bytes. Do we need to add another enum value to libvirt's backing store callback routine, to be used when the header requests data that lies beyond buf_size but is still feasibly valid, for the case where QED designates a backing store location that is beyond 10k but still before the start of the first cluster, rather than the current approach of just treating it as BACKING_STORE_INVALID?
QED doesn't explicitly restrict backing_filename_offset/backing_filename_size to just the header cluster(s). I think it makes sense to say backing_filename_offset + backing_filename_size <= header_size * cluster_size and will add it to the QED spec. But that doesn't guarantee backing_filename_offset < 10 KB. The space after struct QEDHeader is explicitly unstructured so extensions can put arbitrary data there in a backwards compatible way. Stefan
On 11/23/2010 06:26 AM, Stefan Hajnoczi wrote:
More worrying, I don't see anything in QED that requires the filename to occur within the first 10K bytes. Do we need to add another enum value to libvirt's backing store callback routine, to be used when the header requests data that lies beyond buf_size but is still feasibly valid, for the case where QED designates a backing store location that is beyond 10k but still before the start of the first cluster, rather than the current approach of just treating it as BACKING_STORE_INVALID?
QED doesn't explicitly restrict backing_filename_offset/backing_filename_size to just the header cluster(s). I think it makes sense to say backing_filename_offset + backing_filename_size <= header_size * cluster_size and will add it to the QED spec.
Thanks. Do you also want to require that backing_filename_offset > sizeof(backing_filename_size)+offsetof(header,backing_filename_size)? That is, it doesn't make sense for the backing filename to overlap with existing header elements.
But that doesn't guarantee backing_filename_offset < 10 KB. The space after struct QEDHeader is explicitly unstructured so extensions can put arbitrary data there in a backwards compatible way.
I agree that QED should not have to arbitrarily restrict things because of libvirt. Rather, libvirt should be patched to be able to find a backing filename that lies beyond 10K but still within the header clusters. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Nov 23, 2010 at 3:10 PM, Eric Blake <eblake@redhat.com> wrote:
On 11/23/2010 06:26 AM, Stefan Hajnoczi wrote:
More worrying, I don't see anything in QED that requires the filename to occur within the first 10K bytes. Do we need to add another enum value to libvirt's backing store callback routine, to be used when the header requests data that lies beyond buf_size but is still feasibly valid, for the case where QED designates a backing store location that is beyond 10k but still before the start of the first cluster, rather than the current approach of just treating it as BACKING_STORE_INVALID?
QED doesn't explicitly restrict backing_filename_offset/backing_filename_size to just the header cluster(s). I think it makes sense to say backing_filename_offset + backing_filename_size <= header_size * cluster_size and will add it to the QED spec.
Thanks. Do you also want to require that backing_filename_offset > sizeof(backing_filename_size)+offsetof(header,backing_filename_size)? That is, it doesn't make sense for the backing filename to overlap with existing header elements.
Although I agree that it doesn't make sense to overlap the backing filename with the header fields it isn't possible to check this fully. A backwards compatible QED image could have feature bits enabled for new header fields that an old programs don't know about. At that point the "header structure size" is just an arbitrary number and we can't really guard where the backing filename should be stored. Stefan
participants (4)
-
Adam Litke -
Daniel P. Berrange -
Eric Blake -
Stefan Hajnoczi