[libvirt] [PATCH]: Export the 'label' on block devices
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. This actually turns out to be fairly easy to implement; there are really only a few labels that are in common use, and they all have an easy signature to recognize (but see comments in the code about pc98). This patch implements label detection on block devices in virStorageBackendUpdateVolInfoFD, and hooks it up to the iSCSI backend so it works there. To keep code duplication down, I moved some of the enum's from storage_backend_disk.c into a common place. Note, however, that there is a slight semantic change because of this. Previously, if no label was found on a disk in storage_backend_disk.c, it would always return "dos" as the label type. That's not actually true, though; if it's a completely zeroed disk, for instance, it really just has label type of 'unknown'. This patch changes to the new semantic of 'unknown' for label types we don't understand. I don't think this will be a huge issue for compatibility, but there could be something I'm missing. Otherwise, this patch has been tested by me to work, and now when you do: # virsh vol-dumpxml --pool iscsitest lun-1 you'll get: <volume> ... <target> ... <format type='dos' /> Which should be sufficient for oVirt to do it's detection. Signed-off-by: Chris Lalancette <clalance@redhat.com>
Chris Lalancette <clalance@redhat.com> 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.
This actually turns out to be fairly easy to implement; there are really only a few labels that are in common use, and they all have an easy signature to recognize (but see comments in the code about pc98). This patch implements label detection on block devices in virStorageBackendUpdateVolInfoFD, and hooks it up to the iSCSI backend so it works there.
To keep code duplication down, I moved some of the enum's from storage_backend_disk.c into a common place. Note, however, that there is a slight semantic change because of this. Previously, if no label was found on a disk in storage_backend_disk.c, it would always return "dos" as the label type. That's not actually true, though; if it's a completely zeroed disk, for instance, it really just has label type of 'unknown'. This patch changes to the new semantic of 'unknown' for label types we don't understand. I don't think this will be a huge issue for compatibility, but there could be something I'm missing.
Hi Chris, I like the patch. Some minor suggestions below.
Otherwise, this patch has been tested by me to work, and now when you do:
# virsh vol-dumpxml --pool iscsitest lun-1
you'll get:
<volume> ... <target> ... <format type='dos' />
Which should be sufficient for oVirt to do it's detection.
Signed-off-by: Chris Lalancette <clalance@redhat.com> Index: src/storage_backend.c =================================================================== 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 07:29:46 -0000 @@ -192,6 +192,30 @@ return ret; }
+struct diskType disk_types[] = { + { "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 },
Some partition tables are deliberately created to be dual GPT/DOS-based. Here, it gets the first match (as you've deliberately ordered them that way, I see). We might need a mechanism to let the caller prefer one or the other. Also, realize that this can be wrong if there was once a GPT table, and it has since been replaced with a DOS one that still happens to have the GPT signature bytes. Or maybe it was dual GPT/DOS long ago but has since been maintained only as DOS (in which case all GPT-related headers could be gone). This suggests that pronouncing a partition table to have type GPT based solely on the first 1KB is a little risky. But it's probably just fine for now.
+ /* + * 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.
I agree. Why not just exclude it?
+ */ + /*{ "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 }, +}; + int virStorageBackendUpdateVolInfoFD(virConnectPtr conn, virStorageVolDefPtr vol, @@ -245,6 +269,40 @@ if (withCapacity) vol->capacity = end; }
+ /* make sure to set the target format "unknown" to begin with */ + vol->target.format = VIR_STORAGE_POOL_DISK_UNKNOWN; + + if (S_ISBLK(sb.st_mode)) { + off_t start; + int i; + unsigned char buffer[1024]; + ssize_t bytes; + + start = lseek(fd, 0, SEEK_SET); + if (start < 0) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot seek to beginning of file '%s':%s"), + vol->target.path, strerror(errno)); + return -1; + } + memset(buffer, 0, 1024);
You can remove the above memset, given one of the suggestions below.
+ bytes = saferead(fd, buffer, 1024);
Better not to repeat constants like that. use sizeof: bytes = saferead(fd, buffer, sizeof buffer);
+ if (bytes < 0) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot read beginning of file '%s':%s"), + vol->target.path, strerror(errno)); + return -1; + }
At worst, memset just the probably-empty portion of the buffer that wasn't set by the read: memset(buffer + bytes, 0, sizeof buffer - bytes); However, I prefer to drop the memset altogether and check offset+len against "bytes" below:
+ for (i = 0; disk_types[i].name != NULL; i++) {
It'd be good to check that offset+len is within range, regardless of whether the buffer is zeroed. In case someone decides to shrink buffer or to add a type with magic bytes past the 1KB mark. if (disk_types[i].offset + disk_types[i].length > bytes) continue;
+ if (memcmp(buffer+disk_types[i].offset, &disk_types[i].magic, + disk_types[i].length) == 0) { + vol->target.format = disk_types[i].label; + break; + } + } + } + ... 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 07:29:46 -0000 @@ -56,6 +56,30 @@ VIR_STORAGE_BACKEND_POOL_SOURCE_NAME = (1<<4), };
+enum diskLabel { + VIR_STORAGE_POOL_DISK_DOS = 0,
I like to start with 1 or larger, so that using an uninitialized variable (often 0) is more likely to trigger an error.
+ 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, +}; + +struct diskType { + const char *name; + /* the 'label' terminology comes from parted */
i find "label" (meaning "partition table") terminology confusing, if only because each partition can have a volume-label string. If it weren't so ensconced in Parted's code, directory structure (libparted/labels/*.c) and documentation, I would have excised it long ago. I much prefer "partition table". In a way, I don't mean to make you change names, but I'd hate to see Parted's poorly chosen name propagate any more than it has.
+ enum diskLabel label; + unsigned short offset; + unsigned short length; + unsigned long long magic; +}; +extern struct diskType disk_types[]; + typedef struct _virStorageBackendPoolOptions virStorageBackendPoolOptions; typedef virStorageBackendPoolOptions *virStorageBackendPoolOptionsPtr; struct _virStorageBackendPoolOptions {
Jim Meyering wrote:
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 07:29:46 -0000 @@ -192,6 +192,30 @@ return ret; }
+struct diskType disk_types[] = { + { "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 },
Some partition tables are deliberately created to be dual GPT/DOS-based. Here, it gets the first match (as you've deliberately ordered them that way, I see). We might need a mechanism to let the caller prefer one or the other. Also, realize that this can be wrong if there was once a GPT table, and it has since been replaced with a DOS one that still happens to have the GPT signature bytes. Or maybe it was dual GPT/DOS long ago but has since been maintained only as DOS (in which case all GPT-related headers could be gone). This suggests that pronouncing a partition table to have type GPT based solely on the first 1KB is a little risky. But it's probably just fine for now.
Sigh, yes. As you can probably tell, this code is heavily based off of the implementation in parted. I really just looked at the "probe" functions for each of the label types (more on the label name below), and followed what parted did. In the GPT case, it seems like it just probes the front, but you've pointed out via IRC that other routines do checking (like in the last sector, for the backup signature). Like you say, though, I *think* this will be OK for now; if we really need it, we can add "multiple signature checking" later.
+ /* + * 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.
I agree. Why not just exclude it?
I'd be happy to. I mostly included it because it was already there in the storage_backend_disk implementation, and it was relatively easy to add. If no one else has objections, I'm happy to dump it over the side.
+ if (S_ISBLK(sb.st_mode)) { + off_t start; + int i; + unsigned char buffer[1024]; + ssize_t bytes; + + start = lseek(fd, 0, SEEK_SET); + if (start < 0) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot seek to beginning of file '%s':%s"), + vol->target.path, strerror(errno)); + return -1; + } + memset(buffer, 0, 1024);
You can remove the above memset, given one of the suggestions below.
+ bytes = saferead(fd, buffer, 1024);
Better not to repeat constants like that. use sizeof:
bytes = saferead(fd, buffer, sizeof buffer);
Yeah, good point. Will fix.
+ if (bytes < 0) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot read beginning of file '%s':%s"), + vol->target.path, strerror(errno)); + return -1; + }
At worst, memset just the probably-empty portion of the buffer that wasn't set by the read:
memset(buffer + bytes, 0, sizeof buffer - bytes);
However, I prefer to drop the memset altogether and check offset+len against "bytes" below:
+ for (i = 0; disk_types[i].name != NULL; i++) {
It'd be good to check that offset+len is within range, regardless of whether the buffer is zeroed. In case someone decides to shrink buffer or to add a type with magic bytes past the 1KB mark.
if (disk_types[i].offset + disk_types[i].length > bytes) continue;
Ah, yes, I see. Yeah, I thought about adding additional checking for this, but just didn't do it. Your suggestion is good, I'll do that.
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 07:29:46 -0000 @@ -56,6 +56,30 @@ VIR_STORAGE_BACKEND_POOL_SOURCE_NAME = (1<<4), };
+enum diskLabel { + VIR_STORAGE_POOL_DISK_DOS = 0,
I like to start with 1 or larger, so that using an uninitialized variable (often 0) is more likely to trigger an error.
In general, I agree with you, and if I was writing this part from scratch I would do that. I left it as-is because this is the way it was already done. Actually this goes along with the whole "slightly changing semantics" thing I mentioned for storage_backend_disk. Because the vol pointer is always allocated with VIR_ALLOC (i.e. calloc), it means that this field is always 0. That's why "unknown" partition table types always defaulted to DOS, since DOS was the "0'th" enum entry. If we are going to change the semantic, though, we can bump this up to start at 1, and then the 0 entry will always return "unknown", like I think it should.
+struct diskType { + const char *name; + /* the 'label' terminology comes from parted */
i find "label" (meaning "partition table") terminology confusing, if only because each partition can have a volume-label string. If it weren't so ensconced in Parted's code, directory structure (libparted/labels/*.c) and documentation, I would have excised it long ago. I much prefer "partition table".
In a way, I don't mean to make you change names, but I'd hate to see Parted's poorly chosen name propagate any more than it has.
Understood. I had a hard time coming up with a name, because "partition table" tends to get people thinking about FAT16, Linux, etc (which is the wrong thing; those are partition types, not partition table types, but this whole area is confusing). I thought the parted terminology was somehow standard, so I took that. I guess I'll change it to "enum partTableType part_table", which is more technically correct. -- Chris Lalancette
On Thu, Oct 16, 2008 at 09:43:58AM +0200, 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.
This actually turns out to be fairly easy to implement; there are really only a few labels that are in common use, and they all have an easy signature to recognize (but see comments in the code about pc98). This patch implements label detection on block devices in virStorageBackendUpdateVolInfoFD, and hooks it up to the iSCSI backend so it works there.
To keep code duplication down, I moved some of the enum's from storage_backend_disk.c into a common place. Note, however, that there is a slight semantic change because of this. Previously, if no label was found on a disk in storage_backend_disk.c, it would always return "dos" as the label type. That's not actually true, though; if it's a completely zeroed disk, for instance, it really just has label type of 'unknown'. This patch changes to the new semantic of 'unknown' for label types we don't understand. I don't think this will be a huge issue for compatibility, but there could be something I'm missing.
Otherwise, this patch has been tested by me to work, and now when you do:
Patch looks fine to me +1 I wonder what happens when you hit a block device which had no label or partition table, and has a raw file system on it directly. I sometimes do that by mistake on USB devices but the kernel still manage to handle them ... sometimes. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard wrote:
Patch looks fine to me +1 I wonder what happens when you hit a block device which had no label or partition table, and has a raw file system on it directly. I sometimes do that by mistake on USB devices but the kernel still manage to handle them ... sometimes.
Yeah, interesting point. In a quick test, formatting it with ext3 left the entire first 1KB full of zeros, so that would just show up as "unknown". Formatting as msdos put the "0x55AA" signature at the exact same place as the partition table would, so you would get a false positive there. I don't think that's a huge issue, though; trying to do further operations (like actually reading the partition table) are going to just not work. In general, you can definitely craft a partition table that will fool this code. But further operations will fail, so you will still get error reporting, just delayed error reporting. -- Chris Lalancette
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. -- Chris Lalancette
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 :|
Daniel P. Berrange wrote:
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
Yeah, after changing the DOS type to 1, I now realize that UNKNOWN should be the 0'th entry. I'll change that, change to the VIR_ENUM implementation as you suggest, fix up the fallout, and repost. Thanks, -- Chris Lalancette
"Daniel P. Berrange" <berrange@redhat.com> wrote:
+struct diskType disk_types[] = {
This can be const const - surprised Jim didn't catch this :-)
+struct diskType disk_types[] = {
Had to leave some for you. :-P Now that you mention it, though, that should be "static", too.
+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
Or maybe leave it as = 1, to force explicit definition to an enum value. Then, wherever it's zeroed but not explicitly defined it will show up as undefined (possibly triggering some sort of error), rather than proceeding with a mere UNKNOWN. I guess it depends on the relative worth of being able to initialize with calloc-style allocation vs. cost of explicit definition everywhere.
Daniel P. Berrange wrote:
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)
I've implemented this, and there was quite a bit of fallout from it. In particular, I had to change the signature of poolOptions.formatToString and formatFromString, which required a bunch of changes elsewhere in the storage code. The good news is that we remove quite a bit of very similar hand crafted code, so this patch actually removes more code than it adds. Tested by me as before. -- Chris Lalancette
On Thu, Oct 16, 2008 at 02:35:21PM +0200, Chris Lalancette wrote:
Daniel P. Berrange wrote:
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)
I've implemented this, and there was quite a bit of fallout from it. In particular, I had to change the signature of poolOptions.formatToString and formatFromString, which required a bunch of changes elsewhere in the storage code. The good news is that we remove quite a bit of very similar hand crafted code, so this patch actually removes more code than it adds. Tested by me as before. diff -u -r1.15 storage_backend_fs.c --- a/src/storage_backend_fs.c 13 Oct 2008 16:46:29 -0000 1.15 +++ b/src/storage_backend_fs.c 16 Oct 2008 12:31:23 -0000 @@ -48,7 +48,8 @@ #include "xml.h"
enum { - VIR_STORAGE_POOL_FS_AUTO = 0, + VIR_STORAGE_POOL_FS_UNKNOWN = 0, + VIR_STORAGE_POOL_FS_AUTO = 1,
This shouldn't be added - automatic is intended to be default.
enum { - VIR_STORAGE_VOL_RAW, + VIR_STORAGE_VOL_UNKNOWN = 0, + VIR_STORAGE_VOL_RAW = 1,
Likewise, 'raw' should be the default, with no need for unknown. 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 :|
Daniel P. Berrange wrote:
diff -u -r1.15 storage_backend_fs.c --- a/src/storage_backend_fs.c 13 Oct 2008 16:46:29 -0000 1.15 +++ b/src/storage_backend_fs.c 16 Oct 2008 12:31:23 -0000 @@ -48,7 +48,8 @@ #include "xml.h"
enum { - VIR_STORAGE_POOL_FS_AUTO = 0, + VIR_STORAGE_POOL_FS_UNKNOWN = 0, + VIR_STORAGE_POOL_FS_AUTO = 1,
This shouldn't be added - automatic is intended to be default.
enum { - VIR_STORAGE_VOL_RAW, + VIR_STORAGE_VOL_UNKNOWN = 0, + VIR_STORAGE_VOL_RAW = 1,
OK, hopefully last try. Fixed the above two errors and made the disk_type array static as suggested by Jim. -- Chris Lalancette
On Thu, Oct 16, 2008 at 03:47:26PM +0200, Chris Lalancette wrote:
Daniel P. Berrange wrote:
diff -u -r1.15 storage_backend_fs.c --- a/src/storage_backend_fs.c 13 Oct 2008 16:46:29 -0000 1.15 +++ b/src/storage_backend_fs.c 16 Oct 2008 12:31:23 -0000 @@ -48,7 +48,8 @@ #include "xml.h"
enum { - VIR_STORAGE_POOL_FS_AUTO = 0, + VIR_STORAGE_POOL_FS_UNKNOWN = 0, + VIR_STORAGE_POOL_FS_AUTO = 1,
This shouldn't be added - automatic is intended to be default.
enum { - VIR_STORAGE_VOL_RAW, + VIR_STORAGE_VOL_UNKNOWN = 0, + VIR_STORAGE_VOL_RAW = 1,
OK, hopefully last try. Fixed the above two errors and made the disk_type array static as suggested by Jim.
enum { - VIR_STORAGE_POOL_NETFS_AUTO = 0, + VIR_STORAGE_POOL_NETFS_UNKNOWN = 0, + VIR_STORAGE_POOL_NETFS_AUTO = 1, VIR_STORAGE_POOL_NETFS_NFS, + VIR_STORAGE_POOL_NETFS_LAST, };
I missed that one last time around - no need for "unknown" there. Just fix that when committing - the rest is fine now. 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 :|
Daniel P. Berrange wrote:
enum { - VIR_STORAGE_POOL_NETFS_AUTO = 0, + VIR_STORAGE_POOL_NETFS_UNKNOWN = 0, + VIR_STORAGE_POOL_NETFS_AUTO = 1, VIR_STORAGE_POOL_NETFS_NFS, + VIR_STORAGE_POOL_NETFS_LAST, };
I missed that one last time around - no need for "unknown" there. Just fix that when committing - the rest is fine now.
Committed with this last change in place. -- Chris Lalancette
Chris Lalancette <clalance@redhat.com> wrote:
Daniel P. Berrange wrote:
diff -u -r1.15 storage_backend_fs.c --- a/src/storage_backend_fs.c 13 Oct 2008 16:46:29 -0000 1.15 +++ b/src/storage_backend_fs.c 16 Oct 2008 12:31:23 -0000 @@ -48,7 +48,8 @@ #include "xml.h"
enum { - VIR_STORAGE_POOL_FS_AUTO = 0, + VIR_STORAGE_POOL_FS_UNKNOWN = 0, + VIR_STORAGE_POOL_FS_AUTO = 1,
This shouldn't be added - automatic is intended to be default.
enum { - VIR_STORAGE_VOL_RAW, + VIR_STORAGE_VOL_UNKNOWN = 0, + VIR_STORAGE_VOL_RAW = 1,
OK, hopefully last try. Fixed the above two errors and made the disk_type array static as suggested by Jim.
ACK. Looks fine, applied, and passed smoke test: make check && make syntax-check
Chris Lalancette <clalance@redhat.com> wrote:
Daniel P. Berrange wrote:
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)
I've implemented this, and there was quite a bit of fallout from it. In particular, I had to change the signature of poolOptions.formatToString and formatFromString, which required a bunch of changes elsewhere in the storage code. The good news is that we remove quite a bit of very similar hand crafted code, so this patch actually removes more code than it adds. Tested by me as before.
Lots of good fallout. ACK with a tiny tweak:
Chris Lalancette Index: src/storage_backend.c =================================================================== RCS file: /data/cvs/libvirt/src/storage_backend.c,v retrieving revision 1.21 diff -u -r1.21 storage_backend.c --- a/src/storage_backend.c 5 Sep 2008 12:03:45 -0000 1.21 +++ b/src/storage_backend.c 16 Oct 2008 12:31:23 -0000 @@ -60,6 +60,11 @@ #include "storage_backend_fs.h" #endif
+VIR_ENUM_IMPL(virStorageBackendPartTable, + VIR_STORAGE_POOL_DISK_LAST, + "unknown", "dos", "dvh", "gpt", + "mac", "bsd", "pc98", "sun", "lvm2"); + static virStorageBackendPtr backends[] = { #if WITH_STORAGE_DIR &virStorageBackendDirectory, @@ -192,6 +197,30 @@ return ret; }
+const struct diskType const disk_types[] = {
s/const/static/
+ { VIR_STORAGE_POOL_DISK_LVM2, 0x218, 8, 0x31303020324D564CUL }, + { VIR_STORAGE_POOL_DISK_GPT, 0x200, 8, 0x5452415020494645UL }, + { VIR_STORAGE_POOL_DISK_DVH, 0x0, 4, 0x41A9E50BUL }, + { VIR_STORAGE_POOL_DISK_MAC, 0x0, 2, 0x5245UL }, + { VIR_STORAGE_POOL_DISK_BSD, 0x40, 4, 0x82564557UL }, + { VIR_STORAGE_POOL_DISK_SUN, 0x1fc, 2, 0xBEDAUL },
On Thu, Oct 16, 2008 at 09:43:58AM +0200, Chris Lalancette wrote:
To keep code duplication down, I moved some of the enum's from storage_backend_disk.c into a common place. Note, however, that there is a slight semantic change because of this. Previously, if no label was found on a disk in storage_backend_disk.c, it would always return "dos" as the label type. That's not actually true, though; if it's a completely zeroed disk, for instance, it really just has label type of 'unknown'. This patch changes to the new semantic of 'unknown' for label types we don't understand. I don't think this will be a huge issue for compatibility, but there could be something I'm missing.
I don't see a compatability problem - existing behaviour is a clear bug. 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 :|
participants (4)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering