[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH]: Export the 'label' on block devices



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]