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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list