[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
Cc: Stefan Hajnoczi
Cc: Anthony Liguori
---
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[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
Acked-by: Eric Blake
Cc: Stefan Hajnoczi
Cc: Anthony Liguori
---
src/util/storage_file.c | 78 +-
1 files changed, 76 insertions(+), 2 deletions(-)
diff --git a/src/util/storage_file.c b/src/util/storage_file.c
index 89c2cbe..9d97066 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)
@@ -104,7 +106,12 @@ static int vmdk4GetBackingStore(char **, int *,
#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 (4+4+4+4)
+#define QED_HDR_IMAGE_SIZE (QED_HDR_FEATURES_OFFSET+8+8+8+8)
+#define QED_HDR_BACKING_FILE_OFFSET (QED_HDR_IMAGE_SIZE+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
/* VMDK needs at least this to find backing store,
* other formats need less */
@@ -157,7 +164,7 @@ static struct FileTypeInfo const fileTypeInfo[] = {
/* http://wiki.qemu.org/Features/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,
@@ -417,6 +424,73 @@ 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 (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 = VIR_STORAGE_FILE_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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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
> Cc: Stefan Hajnoczi
> Cc: Anthony Liguori
> ---
> 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 ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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 ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] Support for probing qed image metadata
On Fri, Nov 19, 2010 at 11:54 PM, Eric Blake 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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 ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] Support for probing qed image metadata
On Tue, Nov 23, 2010 at 3:10 PM, Eric Blake 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
