[libvirt] [PATCH v2 0/2] Add VDI image format
This is basically v2 for: https://www.redhat.com/archives/libvir-list/2013-February/msg00151.html Very small series, so there's not much more to say ;) Martin Kletzander (2): Support shifted magic in storage files Add basic support for VDI images src/util/virstoragefile.c | 70 ++++++++++++++++++++++++++++------------------- src/util/virstoragefile.h | 3 +- 2 files changed, 44 insertions(+), 29 deletions(-) -- 1.8.1.2
Some files have the magic shifted to some offset other than 0, so we have to support that. I also cleaned up some lines to be more readable and added missing magic for iso file format. --- src/util/virstoragefile.c | 55 ++++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cdac5b1..4f31572 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1,7 +1,7 @@ /* * virstoragefile.c: file utility functions for FS storage backend * - * Copyright (C) 2007-2012 Red Hat, Inc. + * Copyright (C) 2007-2013 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -66,6 +66,7 @@ enum { /* Either 'magic' or 'extension' *must* be provided */ struct FileTypeInfo { + int magicOffset; /* Byte offset of the magic */ const char *magic; /* Optional string of file magic * to check at head of file */ const char *extension; /* Optional file extension to check */ @@ -127,71 +128,75 @@ qedGetBackingStore(char **, int *, const unsigned char *, size_t); static struct FileTypeInfo const fileTypeInfo[] = { - [VIR_STORAGE_FILE_NONE] = { NULL, NULL, LV_LITTLE_ENDIAN, + [VIR_STORAGE_FILE_NONE] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL }, - [VIR_STORAGE_FILE_RAW] = { NULL, NULL, LV_LITTLE_ENDIAN, + [VIR_STORAGE_FILE_RAW] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL }, - [VIR_STORAGE_FILE_DIR] = { NULL, NULL, LV_LITTLE_ENDIAN, + [VIR_STORAGE_FILE_DIR] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL }, [VIR_STORAGE_FILE_BOCHS] = { - /*"Bochs Virtual HD Image", */ /* Untested */ NULL, - NULL, + /*"Bochs Virtual HD Image", */ /* Untested */ + 0, NULL, NULL, LV_LITTLE_ENDIAN, 64, 0x20000, 32+16+16+4+4+4+4+4, 8, 1, -1, NULL }, [VIR_STORAGE_FILE_CLOOP] = { - /*"#!/bin/sh\n#V2.0 Format\nmodprobe cloop file=$0 && mount -r -t iso9660 /dev/cloop $1\n", */ /* Untested */ NULL, - NULL, + /* #!/bin/sh + #V2.0 Format + modprobe cloop file=$0 && mount -r -t iso9660 /dev/cloop $1 + */ /* Untested */ + 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, -1, 0, 0, -1, NULL }, [VIR_STORAGE_FILE_COW] = { - "OOOM", NULL, + 0, "OOOM", NULL, LV_BIG_ENDIAN, 4, 2, 4+4+1024+4, 8, 1, -1, cowGetBackingStore }, [VIR_STORAGE_FILE_DMG] = { - NULL, /* XXX QEMU says there's no magic for dmg, but we should check... */ - ".dmg", + /* XXX QEMU says there's no magic for dmg, + * /usr/share/misc/magic says there is double magic both has + * to be true) and has it disabled as well */ + 0, NULL, ".dmg", 0, -1, 0, -1, 0, 0, -1, NULL }, [VIR_STORAGE_FILE_ISO] = { - NULL, /* XXX there's probably some magic for iso we can validate too... */ - ".iso", + 32769, "CD001", ".iso", 0, -1, 0, -1, 0, 0, -1, NULL }, [VIR_STORAGE_FILE_QCOW] = { - "QFI", NULL, + 0, "QFI", NULL, LV_BIG_ENDIAN, 4, 1, QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW1_HDR_CRYPT, qcow1GetBackingStore, }, [VIR_STORAGE_FILE_QCOW2] = { - "QFI", NULL, + 0, "QFI", NULL, LV_BIG_ENDIAN, 4, 2, QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW2_HDR_CRYPT, qcow2GetBackingStore, }, [VIR_STORAGE_FILE_QED] = { /* http://wiki.qemu.org/Features/QED */ - "QED", NULL, + 0, "QED", NULL, LV_LITTLE_ENDIAN, -2, -1, QED_HDR_IMAGE_SIZE, 8, 1, -1, qedGetBackingStore, }, [VIR_STORAGE_FILE_VMDK] = { - "KDMV", NULL, + 0, "KDMV", NULL, LV_LITTLE_ENDIAN, 4, 1, 4+4+4, 8, 512, -1, vmdk4GetBackingStore }, [VIR_STORAGE_FILE_VPC] = { - "conectix", NULL, + 0, "conectix", NULL, LV_BIG_ENDIAN, 12, 0x10000, 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, -1, NULL }, /* Not direct file formats, but used for various drivers */ - [VIR_STORAGE_FILE_FAT] = { NULL, NULL, LV_LITTLE_ENDIAN, + [VIR_STORAGE_FILE_FAT] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL }, - [VIR_STORAGE_FILE_VHD] = { NULL, NULL, LV_LITTLE_ENDIAN, + [VIR_STORAGE_FILE_VHD] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL }, }; verify(ARRAY_CARDINALITY(fileTypeInfo) == VIR_STORAGE_FILE_LAST); @@ -571,16 +576,18 @@ virStorageFileMatchesMagic(int format, size_t buflen) { int mlen; + int magicOffset = fileTypeInfo[format].magicOffset; + const char *magic = fileTypeInfo[format].magic; - if (fileTypeInfo[format].magic == NULL) + if (magic == NULL) return false; /* Validate magic data */ - mlen = strlen(fileTypeInfo[format].magic); - if (mlen > buflen) + mlen = strlen(magic); + if (magicOffset + mlen > buflen) return false; - if (memcmp(buf, fileTypeInfo[format].magic, mlen) != 0) + if (memcmp(buf + magicOffset, magic, mlen) != 0) return false; return true; -- 1.8.1.2
On 02/04/2013 02:49 PM, Martin Kletzander wrote:
Some files have the magic shifted to some offset other than 0, so we have to support that. I also cleaned up some lines to be more readable and added missing magic for iso file format. --- src/util/virstoragefile.c | 55 ++++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 24 deletions(-)
[VIR_STORAGE_FILE_DMG] = { - NULL, /* XXX QEMU says there's no magic for dmg, but we should check... */ - ".dmg", + /* XXX QEMU says there's no magic for dmg, + * /usr/share/misc/magic says there is double magic both has + * to be true) and has it disabled as well */
This comment now has closing ')' with no open '('. Did you mean something more like: /* XXX QEMU says there's no magic for dmg, * /usr/share/misc/magic lists double magic (both offsets would * have to match) but then disables that check. */ ACK with the comment touched up. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/04/2013 02:49 PM, Martin Kletzander wrote:
Some files have the magic shifted to some offset other than 0, so we have to support that. I also cleaned up some lines to be more readable and added missing magic for iso file format. --- src/util/virstoragefile.c | 55 ++++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 24 deletions(-)
[VIR_STORAGE_FILE_ISO] = { - NULL, /* XXX there's probably some magic for iso we can validate too... */ - ".iso", + 32769, "CD001", ".iso",
Oh, I didn't notice that this wouldn't work until I saw patch 2; you need to hoist a hunk of that patch into this one. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
QEMU is fully capable of handling VDI images and we just refuse to work with them. As qemu-img knows and supports this, there should be no problem with this addition. This is of course, just basic functionality, without searching for any backing files, etc. --- src/util/virstoragefile.c | 15 +++++++++++---- src/util/virstoragefile.h | 3 ++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 4f31572..2700a89 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -51,7 +51,7 @@ VIR_ENUM_IMPL(virStorageFileFormat, "raw", "dir", "bochs", "cloop", "cow", "dmg", "iso", "qcow", "qcow2", "qed", "vmdk", "vpc", - "fat", "vhd") + "fat", "vhd", "vdi") enum lv_endian { LV_LITTLE_ENDIAN = 1, /* 1234 */ @@ -122,9 +122,10 @@ qedGetBackingStore(char **, int *, const unsigned char *, size_t); #define QED_F_BACKING_FILE 0x01 #define QED_F_BACKING_FORMAT_NO_PROBE 0x04 -/* VMDK needs at least this to find backing store, +/* VMDK needs at least 20*512 B to find backing store, + * ISO has 5 Byte magic on offset 32769, * other formats need less */ -#define STORAGE_MAX_HEAD (20*512) +#define STORAGE_MAX_HEAD 32769+5 static struct FileTypeInfo const fileTypeInfo[] = { @@ -164,7 +165,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { }, [VIR_STORAGE_FILE_ISO] = { 32769, "CD001", ".iso", - 0, -1, 0, + LV_LITTLE_ENDIAN, -2, 0, -1, 0, 0, -1, NULL }, [VIR_STORAGE_FILE_QCOW] = { @@ -193,6 +194,12 @@ static struct FileTypeInfo const fileTypeInfo[] = { LV_BIG_ENDIAN, 12, 0x10000, 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, -1, NULL }, + /* TODO: add getBackingStore function */ + [VIR_STORAGE_FILE_VDI] = { + 64, "\x7f\x10\xda\xbe", ".vdi", + LV_LITTLE_ENDIAN, 68, 0x00010001, + 68 + 5 * 4 + 256 + 7 * 4, 8, 1, -1, NULL}, + /* Not direct file formats, but used for various drivers */ [VIR_STORAGE_FILE_FAT] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL }, diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 618b028..83dc3f2 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -1,7 +1,7 @@ /* * virstoragefile.h: file utility functions for FS storage backend * - * Copyright (C) 2007-2009, 2012 Red Hat, Inc. + * Copyright (C) 2007-2009, 2012, 2013 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -44,6 +44,7 @@ enum virStorageFileFormat { VIR_STORAGE_FILE_VPC, VIR_STORAGE_FILE_FAT, VIR_STORAGE_FILE_VHD, + VIR_STORAGE_FILE_VDI, VIR_STORAGE_FILE_LAST, }; -- 1.8.1.2
On 02/04/2013 02:49 PM, Martin Kletzander wrote:
QEMU is fully capable of handling VDI images and we just refuse to work with them. As qemu-img knows and supports this, there should be no problem with this addition.
This is of course, just basic functionality, without searching for any backing files, etc. --- src/util/virstoragefile.c | 15 +++++++++++---- src/util/virstoragefile.h | 3 ++- 2 files changed, 13 insertions(+), 5 deletions(-)
@@ -122,9 +122,10 @@ qedGetBackingStore(char **, int *, const unsigned char *, size_t); #define QED_F_BACKING_FILE 0x01 #define QED_F_BACKING_FORMAT_NO_PROBE 0x04
-/* VMDK needs at least this to find backing store, +/* VMDK needs at least 20*512 B to find backing store, + * ISO has 5 Byte magic on offset 32769, * other formats need less */ -#define STORAGE_MAX_HEAD (20*512) +#define STORAGE_MAX_HEAD 32769+5
This hunk belongs in patch 1/2.
static struct FileTypeInfo const fileTypeInfo[] = { @@ -164,7 +165,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { }, [VIR_STORAGE_FILE_ISO] = { 32769, "CD001", ".iso", - 0, -1, 0, + LV_LITTLE_ENDIAN, -2, 0, -1, 0, 0, -1, NULL
And this one, too.
}, + /* TODO: add getBackingStore function */ + [VIR_STORAGE_FILE_VDI] = { + 64, "\x7f\x10\xda\xbe", ".vdi", + LV_LITTLE_ENDIAN, 68, 0x00010001, + 68 + 5 * 4 + 256 + 7 * 4, 8, 1, -1, NULL}, +
Looks okay.
/* Not direct file formats, but used for various drivers */ [VIR_STORAGE_FILE_FAT] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL }, diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 618b028..83dc3f2 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -1,7 +1,7 @@ /* * virstoragefile.h: file utility functions for FS storage backend * - * Copyright (C) 2007-2009, 2012 Red Hat, Inc. + * Copyright (C) 2007-2009, 2012, 2013 Red Hat, Inc.
I'd use 2012-2013, instead of comma notation. ACK with the reshuffle of iso hunks and copyright fix. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/04/2013 11:06 PM, Eric Blake wrote:
On 02/04/2013 02:49 PM, Martin Kletzander wrote:
QEMU is fully capable of handling VDI images and we just refuse to work with them. As qemu-img knows and supports this, there should be no problem with this addition.
This is of course, just basic functionality, without searching for any backing files, etc. --- src/util/virstoragefile.c | 15 +++++++++++---- src/util/virstoragefile.h | 3 ++- 2 files changed, 13 insertions(+), 5 deletions(-)
@@ -122,9 +122,10 @@ qedGetBackingStore(char **, int *, const unsigned char *, size_t); #define QED_F_BACKING_FILE 0x01 #define QED_F_BACKING_FORMAT_NO_PROBE 0x04
-/* VMDK needs at least this to find backing store, +/* VMDK needs at least 20*512 B to find backing store, + * ISO has 5 Byte magic on offset 32769, * other formats need less */ -#define STORAGE_MAX_HEAD (20*512) +#define STORAGE_MAX_HEAD 32769+5
This hunk belongs in patch 1/2.
static struct FileTypeInfo const fileTypeInfo[] = { @@ -164,7 +165,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { }, [VIR_STORAGE_FILE_ISO] = { 32769, "CD001", ".iso", - 0, -1, 0, + LV_LITTLE_ENDIAN, -2, 0, -1, 0, 0, -1, NULL
And this one, too.
}, + /* TODO: add getBackingStore function */ + [VIR_STORAGE_FILE_VDI] = { + 64, "\x7f\x10\xda\xbe", ".vdi", + LV_LITTLE_ENDIAN, 68, 0x00010001, + 68 + 5 * 4 + 256 + 7 * 4, 8, 1, -1, NULL}, +
Looks okay.
/* Not direct file formats, but used for various drivers */ [VIR_STORAGE_FILE_FAT] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL }, diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 618b028..83dc3f2 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -1,7 +1,7 @@ /* * virstoragefile.h: file utility functions for FS storage backend * - * Copyright (C) 2007-2009, 2012 Red Hat, Inc. + * Copyright (C) 2007-2009, 2012, 2013 Red Hat, Inc.
I'd use 2012-2013, instead of comma notation.
I haven't noticed that. I guess 'copyright-update uses ranges only when updating a range already.
ACK with the reshuffle of iso hunks and copyright fix.
Did all that and pushed both patches, thanks. Martin
participants (2)
-
Eric Blake -
Martin Kletzander