10:18 a.m.
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
10:18 a.m.
New subject: [libvirt] [PATCH 1/4] Allow probing of image formats without version information
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(a)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
4:53 p.m.
New subject: [libvirt] [PATCH 1/4] Allow probing of image formats without version information
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:18 a.m.
New subject: [libvirt] [PATCH 2/4] QED: Basic support for QED images
Add an entry in fileTypeInfo for QED image files.
Signed-off-by: Adam Litke <agl(a)us.ibm.com>
Cc: Stefan Hajnoczi <stefan.hajnoczi(a)uk.ibm.com>
Cc: Anthony Liguori <aliguori(a)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
5:05 p.m.
New subject: [libvirt] [PATCH 2/4] QED: Basic support for QED images
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(a)us.ibm.com>
Cc: Stefan Hajnoczi <stefan.hajnoczi(a)uk.ibm.com>
Cc: Anthony Liguori <aliguori(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:16 p.m.
New subject: [libvirt] [PATCH 2/4] QED: Basic support for QED images
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
10:18 a.m.
New subject: [libvirt] [PATCH 3/4] storage_file: Add a new flag to mark backing files that are safe to probe
Signed-off-by: Adam Litke <agl(a)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
5:06 p.m.
New subject: [libvirt] [PATCH 3/4] storage_file: Add a new flag to mark backing files that are safe to probe
On 11/19/2010 09:18 AM, Adam Litke wrote:
Signed-off-by: Adam Litke <agl(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:04 a.m.
New subject: [libvirt] [PATCH 3/4] storage_file: Add a new flag to mark backing files that are safe to probe
On Fri, Nov 19, 2010 at 10:18:17AM -0600, Adam Litke wrote:
Signed-off-by: Adam Litke <agl(a)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
8:14 a.m.
New subject: [libvirt] [PATCH 3/4] storage_file: Add a new flag to mark backing files that are safe to probe
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
10:18 a.m.
New subject: [libvirt] [PATCH 4/4] Support for probing qed image metadata
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(a)us.ibm.com>
Cc: Stefan Hajnoczi <stefan.hajnoczi(a)uk.ibm.com>
Cc: Anthony Liguori <aliguori(a)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
5:32 p.m.
New subject: [libvirt] [PATCH 4/4] Support for probing qed image metadata
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(a)us.ibm.com>
Cc: Stefan Hajnoczi <stefan.hajnoczi(a)uk.ibm.com>
Cc: Anthony Liguori <aliguori(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:54 p.m.
New subject: [libvirt] [PATCH 4/4] Support for probing qed image metadata
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
7:26 a.m.
New subject: [libvirt] [PATCH 4/4] Support for probing qed image metadata
On Fri, Nov 19, 2010 at 11:54 PM, Eric Blake <eblake(a)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
9:10 a.m.
New subject: [libvirt] [PATCH 4/4] Support for probing qed image metadata
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:42 a.m.
New subject: [libvirt] [PATCH 4/4] Support for probing qed image metadata
On Tue, Nov 23, 2010 at 3:10 PM, Eric Blake <eblake(a)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
5113
days inactive
5117
days old
15 comments
4 participants
participants (4)
-
Adam Litke -
Daniel P. Berrange -
Eric Blake -
Stefan Hajnoczi