[libvirt] [PATCH 0/4] Storage disk backend fixes
The following series fixes some issues in the disk storage backend, most wrt the recently added ability to create disk volumes. Cole Robinson (4): storage: disk: Fix parthelper '-g' option handling. storage: disk: Fix segfault creating volume without target path storage: disk: Default to 'ext2' for new volumes. storage: disk: Use capacity, not allocation, when creating volume. src/parthelper.c | 4 ++-- src/storage_backend_disk.c | 33 +++++++++++++++++++-------------- src/storage_conf.c | 2 +- 3 files changed, 22 insertions(+), 17 deletions(-)
Typo was breaking 'parthelper -g', preventing disk pool definition. --- src/parthelper.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/parthelper.c b/src/parthelper.c index f456ccc..5df46e8 100644 --- a/src/parthelper.c +++ b/src/parthelper.c @@ -36,7 +36,7 @@ #include <stdio.h> /* we don't need to include the full internal.h just for this */ -#define STRNEQ(a,b) (strcmp((a),(b)) != 0) +#define STREQ(a,b) (strcmp((a),(b)) == 0) /* Make the comparisons below fail if your parted headers are so old that they lack the definition. */ @@ -56,7 +56,7 @@ int main(int argc, char **argv) PedPartition *part; int cmd = DISK_LAYOUT; - if (argc == 3 && STRNEQ(argv[2], "-g")) { + if (argc == 3 && STREQ(argv[2], "-g")) { cmd = DISK_GEOMETRY; } else if (argc != 2) { fprintf(stderr, "syntax: %s DEVICE [-g]\n", argv[0]); -- 1.6.0.6
On Fri, Jul 10, 2009 at 03:32:20PM -0400, Cole Robinson wrote:
Typo was breaking 'parthelper -g', preventing disk pool definition. --- src/parthelper.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/parthelper.c b/src/parthelper.c index f456ccc..5df46e8 100644 --- a/src/parthelper.c +++ b/src/parthelper.c @@ -36,7 +36,7 @@ #include <stdio.h>
/* we don't need to include the full internal.h just for this */ -#define STRNEQ(a,b) (strcmp((a),(b)) != 0) +#define STREQ(a,b) (strcmp((a),(b)) == 0)
Dubious optimization to re-define this here, we should just include the real internal.h
/* Make the comparisons below fail if your parted headers are so old that they lack the definition. */ @@ -56,7 +56,7 @@ int main(int argc, char **argv) PedPartition *part; int cmd = DISK_LAYOUT;
- if (argc == 3 && STRNEQ(argv[2], "-g")) { + if (argc == 3 && STREQ(argv[2], "-g")) { cmd = DISK_GEOMETRY; } else if (argc != 2) { fprintf(stderr, "syntax: %s DEVICE [-g]\n", argv[0]);
ACK 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 :|
On Fri, Jul 10, 2009 at 03:32:20PM -0400, Cole Robinson wrote:
Typo was breaking 'parthelper -g', preventing disk pool definition. --- src/parthelper.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/parthelper.c b/src/parthelper.c index f456ccc..5df46e8 100644 --- a/src/parthelper.c +++ b/src/parthelper.c @@ -36,7 +36,7 @@ #include <stdio.h>
/* we don't need to include the full internal.h just for this */ -#define STRNEQ(a,b) (strcmp((a),(b)) != 0) +#define STREQ(a,b) (strcmp((a),(b)) == 0)
/* Make the comparisons below fail if your parted headers are so old that they lack the definition. */ @@ -56,7 +56,7 @@ int main(int argc, char **argv) PedPartition *part; int cmd = DISK_LAYOUT;
- if (argc == 3 && STRNEQ(argv[2], "-g")) { + if (argc == 3 && STREQ(argv[2], "-g")) { cmd = DISK_GEOMETRY; } else if (argc != 2) { fprintf(stderr, "syntax: %s DEVICE [-g]\n", argv[0]);
Oops, that's me :-\ ACK W.r.t. redefinition, well it was looking like including the full internal.h was way beyond the scope of that small check. 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/
Remove unneeded target path duplication, which could carelessly dereference NULL. Make it clear where 'key' is actually filled in. --- src/storage_backend_disk.c | 33 +++++++++++++++++++-------------- 1 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/storage_backend_disk.c b/src/storage_backend_disk.c index 1eb3eac..e50825b 100644 --- a/src/storage_backend_disk.c +++ b/src/storage_backend_disk.c @@ -226,10 +226,18 @@ virStorageBackendDiskMakeVol(virConnectPtr conn, if (STREQ(groups[2], "metadata") || STREQ(groups[2], "data")) { virStorageVolDefPtr vol = data; - /* We're searching for a specific vol only, so ignore others */ - if (vol && - STRNEQ(vol->name, groups[0])) - return 0; + + if (vol) { + /* We're searching for a specific vol only */ + if (vol->key) { + if (STRNEQ(vol->key, groups[0])) + return 0; + } else if (virStorageVolDefFindByKey(pool, groups[0]) != NULL) { + /* If no key, the volume must be newly created. If groups[0] + * isn't already a volume, assume it's the path we want */ + return 0; + } + } return virStorageBackendDiskMakeDataVol(conn, pool, groups, vol); } else if (STREQ(groups[2], "free")) { @@ -553,15 +561,9 @@ virStorageBackendDiskCreateVol(virConnectPtr conn, return -1; } - if (vol->key == NULL) { - /* XXX base off a unique key of the underlying disk */ - if ((vol->key = strdup(vol->target.path)) == NULL) { - virReportOOMError(conn); - return -1; - } - } - - if(virStorageBackendDiskPartBoundries(conn, pool, &startOffset, &endOffset, vol->allocation) != 0) { + if (virStorageBackendDiskPartBoundries(conn, pool, &startOffset, + &endOffset, + vol->allocation) != 0) { return -1; } @@ -580,7 +582,10 @@ virStorageBackendDiskCreateVol(virConnectPtr conn, VIR_FREE(pool->def->source.devices[0].freeExtents); pool->def->source.devices[0].nfreeExtent = 0; - /* Fetch actual extent info */ + /* Specifying a target path is meaningless */ + VIR_FREE(vol->target.path); + + /* Fetch actual extent info, generate key */ if (virStorageBackendDiskReadPartitions(conn, pool, vol) < 0) return -1; -- 1.6.0.6
On Fri, Jul 10, 2009 at 03:32:21PM -0400, Cole Robinson wrote:
Remove unneeded target path duplication, which could carelessly dereference NULL. Make it clear where 'key' is actually filled in. --- src/storage_backend_disk.c | 33 +++++++++++++++++++-------------- 1 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/src/storage_backend_disk.c b/src/storage_backend_disk.c index 1eb3eac..e50825b 100644 --- a/src/storage_backend_disk.c +++ b/src/storage_backend_disk.c @@ -226,10 +226,18 @@ virStorageBackendDiskMakeVol(virConnectPtr conn, if (STREQ(groups[2], "metadata") || STREQ(groups[2], "data")) { virStorageVolDefPtr vol = data; - /* We're searching for a specific vol only, so ignore others */ - if (vol && - STRNEQ(vol->name, groups[0])) - return 0; + + if (vol) { + /* We're searching for a specific vol only */ + if (vol->key) { + if (STRNEQ(vol->key, groups[0])) + return 0; + } else if (virStorageVolDefFindByKey(pool, groups[0]) != NULL) { + /* If no key, the volume must be newly created. If groups[0] + * isn't already a volume, assume it's the path we want */ + return 0; + } + }
return virStorageBackendDiskMakeDataVol(conn, pool, groups, vol); } else if (STREQ(groups[2], "free")) { @@ -553,15 +561,9 @@ virStorageBackendDiskCreateVol(virConnectPtr conn, return -1; }
- if (vol->key == NULL) { - /* XXX base off a unique key of the underlying disk */ - if ((vol->key = strdup(vol->target.path)) == NULL) { - virReportOOMError(conn); - return -1; - } - } - - if(virStorageBackendDiskPartBoundries(conn, pool, &startOffset, &endOffset, vol->allocation) != 0) { + if (virStorageBackendDiskPartBoundries(conn, pool, &startOffset, + &endOffset, + vol->allocation) != 0) { return -1; }
@@ -580,7 +582,10 @@ virStorageBackendDiskCreateVol(virConnectPtr conn, VIR_FREE(pool->def->source.devices[0].freeExtents); pool->def->source.devices[0].nfreeExtent = 0;
- /* Fetch actual extent info */ + /* Specifying a target path is meaningless */ + VIR_FREE(vol->target.path); + + /* Fetch actual extent info, generate key */ if (virStorageBackendDiskReadPartitions(conn, pool, vol) < 0) return -1;
ACK 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 :|
On Fri, Jul 10, 2009 at 03:32:21PM -0400, Cole Robinson wrote:
Remove unneeded target path duplication, which could carelessly dereference NULL. Make it clear where 'key' is actually filled in.
Looks fine, ACK, 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/
Currently, if no format is specified for a new disk volume, we pass the invalid value "none" as the FS type to 'parted mkpart'. There doesn't seem to be a way to have parted not format the drive, so just default to using 'ext2' in this case: this shouldn't cause any harm, since we are creating a new partition in the first place. --- src/storage_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/storage_conf.c b/src/storage_conf.c index fe1dc90..075279c 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -95,7 +95,7 @@ VIR_ENUM_IMPL(virStorageVolFormatFileSystem, VIR_ENUM_IMPL(virStoragePartedFsType, VIR_STORAGE_PARTED_FS_TYPE_LAST, - "none", "ext2", "fat16", + "ext2", "ext2", "fat16", "fat32", "linux-swap", "ext2", "ext2", "extended") -- 1.6.0.6
On Fri, Jul 10, 2009 at 03:32:22PM -0400, Cole Robinson wrote:
Currently, if no format is specified for a new disk volume, we pass the invalid value "none" as the FS type to 'parted mkpart'.
There doesn't seem to be a way to have parted not format the drive, so just default to using 'ext2' in this case: this shouldn't cause any harm, since we are creating a new partition in the first place. --- src/storage_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage_conf.c b/src/storage_conf.c index fe1dc90..075279c 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -95,7 +95,7 @@ VIR_ENUM_IMPL(virStorageVolFormatFileSystem,
VIR_ENUM_IMPL(virStoragePartedFsType, VIR_STORAGE_PARTED_FS_TYPE_LAST, - "none", "ext2", "fat16", + "ext2", "ext2", "fat16", "fat32", "linux-swap", "ext2", "ext2", "extended")
ACK 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 :|
On Fri, Jul 10, 2009 at 03:32:22PM -0400, Cole Robinson wrote:
Currently, if no format is specified for a new disk volume, we pass the invalid value "none" as the FS type to 'parted mkpart'.
There doesn't seem to be a way to have parted not format the drive, so just default to using 'ext2' in this case: this shouldn't cause any harm, since we are creating a new partition in the first place.
Sounds fine, ACK 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/
There isn't any way to dictate allocation when creating disk volumes, so capacity is the only relevant value. --- src/storage_backend_disk.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/storage_backend_disk.c b/src/storage_backend_disk.c index e50825b..ae2acae 100644 --- a/src/storage_backend_disk.c +++ b/src/storage_backend_disk.c @@ -563,7 +563,7 @@ virStorageBackendDiskCreateVol(virConnectPtr conn, if (virStorageBackendDiskPartBoundries(conn, pool, &startOffset, &endOffset, - vol->allocation) != 0) { + vol->capacity) != 0) { return -1; } -- 1.6.0.6
On Fri, Jul 10, 2009 at 03:32:23PM -0400, Cole Robinson wrote:
There isn't any way to dictate allocation when creating disk volumes, so capacity is the only relevant value. --- src/storage_backend_disk.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage_backend_disk.c b/src/storage_backend_disk.c index e50825b..ae2acae 100644 --- a/src/storage_backend_disk.c +++ b/src/storage_backend_disk.c @@ -563,7 +563,7 @@ virStorageBackendDiskCreateVol(virConnectPtr conn,
if (virStorageBackendDiskPartBoundries(conn, pool, &startOffset, &endOffset, - vol->allocation) != 0) { + vol->capacity) != 0) { return -1; }
ACK 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 :|
On Fri, Jul 10, 2009 at 03:32:23PM -0400, Cole Robinson wrote:
There isn't any way to dictate allocation when creating disk volumes, so capacity is the only relevant value.
Sounds right, ACK, 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:
On Fri, Jul 10, 2009 at 03:32:23PM -0400, Cole Robinson wrote:
There isn't any way to dictate allocation when creating disk volumes, so capacity is the only relevant value.
Sounds right, ACK,
Daniel
Thanks, I've pushed this series now. - Cole
On Fri, Jul 10, 2009 at 03:32:19PM -0400, Cole Robinson wrote:
The following series fixes some issues in the disk storage backend, most wrt the recently added ability to create disk volumes.
I've started adding some tests to the libvirt TCK to cover the storage APIs. So far I've got tests covering creation of all the volume formats in directory pools, and cloning of volumes http://libvirt.org/git/?p=libvirt-tck.git;a=tree;f=scripts/storage;hb=HEAD I'll look at adding coverage for LVM & disk volume creation next so we can validate this functionality better. 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 (3)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard