On Thu, Oct 16, 2008 at 11:52:48AM +0200, Chris Lalancette wrote:
Chris Lalancette wrote:
To support LVM partitioning in oVirt, one of the things we need is the ability to tell what kind of label is currently on a block device. Here, a 'label' is used in the same sense that it is used in parted; namely, it defines which kind of partition table is on the disk, whether it be DOS, LVM2, SUN, BSD, etc. Note that this is different than the partition type; those are things like Linux, FAT16, FAT32, etc.
Updated patch based on jim's feedback. I did not remove the pc98 type; if everyone else thinks it's a good idea, I'll remove it, but it is part of the ABI in some sense. Otherwise, I followed all of jim's suggestions.
I think we can keep it for completeness, since partd supports it, it is conceivable (though unlikely) that we can see it and its not a real burden to keep it.
=================================================================== RCS file: /data/cvs/libvirt/src/storage_backend.c,v retrieving revision 1.21 diff -u -r1.21 storage_backend.c --- src/storage_backend.c 5 Sep 2008 12:03:45 -0000 1.21 +++ src/storage_backend.c 16 Oct 2008 09:49:01 -0000 @@ -192,6 +192,30 @@ return ret; }
+struct diskType disk_types[] = {
This can be const const - surprised Jim didn't catch this :-)
+ { "lvm2", VIR_STORAGE_POOL_DISK_LVM2, 0x218, 8, 0x31303020324D564CUL }, + { "gpt", VIR_STORAGE_POOL_DISK_GPT, 0x200, 8, 0x5452415020494645UL }, + { "dvh", VIR_STORAGE_POOL_DISK_DVH, 0x0, 4, 0x41A9E50BUL }, + { "mac", VIR_STORAGE_POOL_DISK_MAC, 0x0, 2, 0x5245UL }, + { "bsd", VIR_STORAGE_POOL_DISK_BSD, 0x40, 4, 0x82564557UL }, + { "sun", VIR_STORAGE_POOL_DISK_SUN, 0x1fc, 2, 0xBEDAUL }, + /* + * NOTE: pc98 is funky; the actual signature is 0x55AA (just like dos), so + * we can't use that. At the moment I'm relying on the "dummy" IPL + * bootloader data that comes from parted. Luckily, the chances of running + * into a pc98 machine running libvirt are approximately nil. + */ + /*{ "pc98", 0x1fe, 2, 0xAA55UL },*/ + { "pc98", VIR_STORAGE_POOL_DISK_PC98, 0x0, 8, 0x314C5049000000CBUL }, + /* + * NOTE: the order is important here; some other disk types (like GPT and + * and PC98) also have 0x55AA at this offset. For that reason, the DOS + * one must be the last one. + */ + { "dos", VIR_STORAGE_POOL_DISK_DOS, 0x1fe, 2, 0xAA55UL }, + { NULL, -1, 0x0, 0, 0x0UL }, +}; +
Remove the strings from here - they're not required for the probing - only the constant it needed. The string-ification of the types is better handled by our enum support
+int +virStorageBackendDiskLabelFormatFromString(virConnectPtr conn ATTRIBUTE_UNUSED, + const char *format) { + int i; + + if (format == NULL) + return VIR_STORAGE_POOL_DISK_UNKNOWN; + + for (i = 0; disk_types[i].name != NULL; i++) { + if (STREQ(format, disk_types[i].name)) + return disk_types[i].part_table_type; + } + + return VIR_STORAGE_POOL_DISK_UNKNOWN; +} + +const char * +virStorageBackendDiskLabelFormatToString(virConnectPtr conn ATTRIBUTE_UNUSED, + int format) { + int i; + + for (i = 0; disk_types[i].name != NULL; i++) { + if (format == disk_types[i].part_table_type) + return disk_types[i].name; + } + + return "unknown"; +}
I know you're just replicating the existing code, but both these functions can be killed off and replaced with auto-generated code from our enum support macros VIR_ENUM_IMPL(virStorageBackendDiskLabel, VIR_STORAGE_POOL_DISK_LAST, "dos", "dvh", "gpt", "mac", "bsd", "pc98", "sun", "lvm2" "unknown") And in the header file just VIR_ENUM_DECL(virStorageBackendDiskLabel)
#ifndef __MINGW32__ /* Index: src/storage_backend.h =================================================================== RCS file: /data/cvs/libvirt/src/storage_backend.h,v retrieving revision 1.7 diff -u -r1.7 storage_backend.h --- src/storage_backend.h 2 Sep 2008 14:15:42 -0000 1.7 +++ src/storage_backend.h 16 Oct 2008 09:49:01 -0000 @@ -56,6 +56,29 @@ VIR_STORAGE_BACKEND_POOL_SOURCE_NAME = (1<<4), };
+enum partTableType { + VIR_STORAGE_POOL_DISK_DOS = 1, + VIR_STORAGE_POOL_DISK_DVH, + VIR_STORAGE_POOL_DISK_GPT, + VIR_STORAGE_POOL_DISK_MAC, + VIR_STORAGE_POOL_DISK_BSD, + VIR_STORAGE_POOL_DISK_PC98, + VIR_STORAGE_POOL_DISK_SUN, + VIR_STORAGE_POOL_DISK_LVM2, + /* the "unknown" disk is 1 billion (and not, for instance, -1), to make + sure it doesn't run afoul of error checking */ + VIR_STORAGE_POOL_DISK_UNKNOWN = 1 * 1024 * 1024 * 1024,
I don't understand why it has to be 1-billion ? For the enum support to work correctly, we need this to be contiguous, starting from zero, and and as the last element have VIR_STORAGE_POOL_DISK_LAST This perhaps suggests that DISK_UNKNOWN should be the first entry so that if you have a zero'd struct with a disk type, it'll get defaulted to UNKOWN Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|