2:43 a.m.
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(a)redhat.com>
3:53 a.m.
Chris Lalancette <clalance(a)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(a)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 {
4:26 a.m.
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
4:26 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:34 a.m.
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
4:52 a.m.
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
5:29 a.m.
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 :|
5:49 a.m.
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
6:11 a.m.
"Daniel P. Berrange" <berrange(a)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.
7:35 a.m.
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
7:43 a.m.
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 :|
8:47 a.m.
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
9:41 a.m.
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 :|
10:05 a.m.
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
9:41 a.m.
Chris Lalancette <clalance(a)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
8:11 a.m.
Chris Lalancette <clalance(a)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 },
5:20 a.m.
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 :|
5884
days inactive
5884
days old
16 comments
4 participants
participants (4)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering