[libvirt] [PATCHv2 0/6] Resolve issues in disk pool backend
https://bugzilla.redhat.com/show_bug.cgi?id=1138516 v1: http://www.redhat.com/archives/libvir-list/2015-January/msg00465.html In my previous attempt to resolve the issue, I created a stateDir and saved the volume XML for each pool in order to attempt to preserve the volume "name" and "target format type". However, it was pointed out during review that having a different "name" was not the original design intention. The intention was the name would be the partition name rather than something the user fabricates and expects to be saved/used. Since partition naming is handled by parted, the control over the name is not ours. Additionally, a pool refresh or libvirtd restart/reload would use the 'default' of the source device path and the partition number from the partition table. The "simple" fix to the issue is in patch 6; however, as I was going through fixing this I realized a few things and found a few other issues along the way, namely: Patches #1 & #2: After creating the partition if we fail something within the callback to virStorageBackendDiskMakeDataVol as a result of parsing the partition table from libvirt_parthelper, then the partition wasn't removed. So, patches 1 & 2 work at moving the DeleteVol code and then calling it if something in virStorageBackendDiskReadPartitions fails. Patch #3: When determining what type of partitions currently exist in the pool we compared against the target.format which is supposed to be the file system format type of the partition on the target rather than whether the partition is a primary, extended, or logical partition on the source. Since we set the partType on the source while reading the partitions, that's what we should use. Patch #4: During a refresh of the pool, we use virStorageBackendDiskReadPartitions in order to determine what types of partitions exist; however, if we have an extended partition and have created either a logical partition within or another primary partition after the extended one, then the open() will fail with ENXIO. So I special cased that. Patch #5: When we delete an extended partition, any logical partitions that existed in the pool would still be listed even though as part of the delete partition logic all the logical partitions were also deleted. Patch #6: So here is essentially the replacement for the previous patch series. Since the theory is one is supposed to know what they are doing and will provide the correct vol->name, make sure that they do so and cause a failure if they don't (indicating what the name should be). As an alternative we could just "overwrite" the name like we did anyway with pool refresh or libvirtd restart/reload by doing the following instead: /* Like the reload/restart/refresh path - use the name created by * parted rather than the API/user provided name */ if (STRNEQ(vol->name, partname)) { VIR_FREE(vol->name); if (VIR_STRDUP(vol->name, partname) < 0) return -1; } I also added details to the virsh.pod and formatstorage.html.in for the 'name' and the intersting "side effect" I found using 'virsh vol-create-as $pool $name $size --format extended'. I'd remove it, but I fear that someone else found this and has made use of it. The reality is that '--format' is supposed to be the file system format of the partition. However, the way it's used in the code will take what is supposed to be target format and allow creation of an extended partition. In virStorageBackendDiskPartFormat, if the pool source.format is DOS and the vol->target.format is VIR_STORAGE_VOL_DISK_EXTENDED, then we create an extended source partition as long as one doesn't already exist. John Ferlan (6): storage: Move virStorageBackendDiskDeleteVol storage: Attempt error recovery in virStorageBackendDiskCreateVol storage: Fix check for partition type for disk backing volumes storage: Adjust how to refresh extended partition disk data storage: When delete extended partition, need to refresh pool storage: Check the partition name against provided name docs/formatstorage.html.in | 12 +- src/storage/storage_backend.c | 4 + src/storage/storage_backend_disk.c | 235 +++++++++++++++++++++++-------------- tools/virsh.pod | 17 ++- 4 files changed, 174 insertions(+), 94 deletions(-) -- 2.1.0
Move the API to before virStorageBackendDiskCreateVol in order to be able to call the DeleteVol API when virStorageBackendDiskReadPartitions fails so that we don't by chance leave a partition on the disk. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_disk.c | 137 +++++++++++++++++++------------------ 1 file changed, 69 insertions(+), 68 deletions(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 3f97fd9..60f8393 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -640,6 +640,75 @@ virStorageBackendDiskPartBoundaries(virStoragePoolObjPtr pool, static int +virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + unsigned int flags) +{ + char *part_num = NULL; + char *devpath = NULL; + char *dev_name, *srcname; + virCommandPtr cmd = NULL; + bool isDevMapperDevice; + int rc = -1; + + virCheckFlags(0, -1); + + if (virFileResolveLink(vol->target.path, &devpath) < 0) { + virReportSystemError(errno, + _("Couldn't read volume target path '%s'"), + vol->target.path); + goto cleanup; + } + + dev_name = last_component(devpath); + srcname = last_component(pool->def->source.devices[0].path); + VIR_DEBUG("dev_name=%s, srcname=%s", dev_name, srcname); + + isDevMapperDevice = virIsDevMapperDevice(devpath); + + if (!isDevMapperDevice && !STRPREFIX(dev_name, srcname)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Volume path '%s' did not start with parent " + "pool source device name."), dev_name); + goto cleanup; + } + + if (!isDevMapperDevice) { + part_num = dev_name + strlen(srcname); + + if (*part_num == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse partition number from target " + "'%s'"), dev_name); + goto cleanup; + } + + /* eg parted /dev/sda rm 2 */ + cmd = virCommandNewArgList(PARTED, + pool->def->source.devices[0].path, + "rm", + "--script", + part_num, + NULL); + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + } else { + cmd = virCommandNewArgList(DMSETUP, "remove", "--force", devpath, NULL); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + } + + rc = 0; + cleanup: + VIR_FREE(devpath); + virCommandFree(cmd); + return rc; +} + + +static int virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, virStorageVolDefPtr vol) @@ -714,74 +783,6 @@ virStorageBackendDiskBuildVolFrom(virConnectPtr conn, return build_func(conn, pool, vol, inputvol, flags); } -static int -virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, - virStoragePoolObjPtr pool, - virStorageVolDefPtr vol, - unsigned int flags) -{ - char *part_num = NULL; - char *devpath = NULL; - char *dev_name, *srcname; - virCommandPtr cmd = NULL; - bool isDevMapperDevice; - int rc = -1; - - virCheckFlags(0, -1); - - if (virFileResolveLink(vol->target.path, &devpath) < 0) { - virReportSystemError(errno, - _("Couldn't read volume target path '%s'"), - vol->target.path); - goto cleanup; - } - - dev_name = last_component(devpath); - srcname = last_component(pool->def->source.devices[0].path); - VIR_DEBUG("dev_name=%s, srcname=%s", dev_name, srcname); - - isDevMapperDevice = virIsDevMapperDevice(devpath); - - if (!isDevMapperDevice && !STRPREFIX(dev_name, srcname)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Volume path '%s' did not start with parent " - "pool source device name."), dev_name); - goto cleanup; - } - - if (!isDevMapperDevice) { - part_num = dev_name + strlen(srcname); - - if (*part_num == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse partition number from target " - "'%s'"), dev_name); - goto cleanup; - } - - /* eg parted /dev/sda rm 2 */ - cmd = virCommandNewArgList(PARTED, - pool->def->source.devices[0].path, - "rm", - "--script", - part_num, - NULL); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - } else { - cmd = virCommandNewArgList(DMSETUP, "remove", "--force", devpath, NULL); - - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - } - - rc = 0; - cleanup: - VIR_FREE(devpath); - virCommandFree(cmd); - return rc; -} - virStorageBackend virStorageBackendDisk = { .type = VIR_STORAGE_POOL_DISK, -- 2.1.0
During virStorageBackendDiskCreateVol if virStorageBackendDiskReadPartitions fails, then we were leaving with an error and a partition on the disk for which there was no corresponding volume and used space on the disk which could be reclaimable through direct parted activity. On a subsequent restart, reload, or refresh the volume may magically appear too. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_disk.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 60f8393..0c4126a 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -654,6 +654,13 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1); + if (!vol->target.path) { + virReportError(VIR_ERR_INVALID_ARG, + _("volume target path empty for source path '%s'"), + pool->def->source.devices[0].path); + return -1; + } + if (virFileResolveLink(vol->target.path, &devpath) < 0) { virReportSystemError(errno, _("Couldn't read volume target path '%s'"), @@ -709,7 +716,7 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, static int -virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendDiskCreateVol(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol) { @@ -756,8 +763,16 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, VIR_FREE(vol->target.path); /* Fetch actual extent info, generate key */ - if (virStorageBackendDiskReadPartitions(pool, vol) < 0) + if (virStorageBackendDiskReadPartitions(pool, vol) < 0) { + /* Best effort to remove the partition. Ignore any errors + * since we could be calling this with vol->target.path == NULL + */ + virErrorPtr save_err = virSaveLastError(); + ignore_value(virStorageBackendDiskDeleteVol(conn, pool, vol, 0)); + virSetError(save_err); + virFreeError(save_err); goto cleanup; + } res = 0; -- 2.1.0
While checking the existing partitions in virStorageBackendDiskPartFormat, the code would erroneously compare the volume target format type (eg, the virStoragePartedFsType) rather than the source partition type (eg, the virStorageVolTypeDisk) which is set during virStorageBackendDiskReadPartitions. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_disk.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 0c4126a..31b6025 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -495,8 +495,8 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, if (vol->target.format == VIR_STORAGE_VOL_DISK_EXTENDED) { /* make sure we don't have a extended partition already */ for (i = 0; i < pool->volumes.count; i++) { - if (pool->volumes.objs[i]->target.format == - VIR_STORAGE_VOL_DISK_EXTENDED) { + if (pool->volumes.objs[i]->source.partType == + VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("extended partition already exists")); return -1; @@ -517,8 +517,8 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, case VIR_STORAGE_VOL_DISK_TYPE_LOGICAL: /* make sure we have a extended partition */ for (i = 0; i < pool->volumes.count; i++) { - if (pool->volumes.objs[i]->target.format == - VIR_STORAGE_VOL_DISK_EXTENDED) { + if (pool->volumes.objs[i]->source.partType == + VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) { if (virAsprintf(partFormat, "logical %s", partedFormat) < 0) return -1; -- 2.1.0
During virStorageBackendDiskMakeDataVol processing, if we find an extended partition, then handle it specially when updating the capacity/allocation rather than calling virStorageBackendUpdateVolInfo. As it turns out, once a logical partition exists, any attempt to refresh the pool or after libvirtd restart/reload will result in a failure to open the extended partition device resulting in the inability to start the pool. The downside to this is we will lose the <permissions> and <timestamps> for the extended partition upon subsequent restart, refresh, reload since the stat() in virStorageBackendUpdateVolTargetInfoFD will not be called. However, since it's really only a container and shouldn't directly be used for storage that seems reasonable. Therefore, only use the existing code that already had a comment about getting the allocation wrong for extended partitions for just the setting of the extended partition data. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 4 ++++ src/storage/storage_backend_disk.c | 32 +++++++++++++++++++++++--------- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index b990a82..d7bccfe 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1385,6 +1385,10 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, VIR_WARN("ignoring missing file '%s'", path); return -2; } + if (errno == ENXIO && noerror) { + VIR_WARN("ignoring missing fifo '%s'", path); + return -2; + } virReportSystemError(errno, _("cannot open volume '%s'"), path); return -1; diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 31b6025..233e293 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -110,11 +110,6 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, return -1; } - /* Refresh allocation/capacity/perms */ - if (virStorageBackendUpdateVolInfo(vol, true, false, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) - return -1; - /* set partition type */ if (STREQ(groups[1], "normal")) vol->source.partType = VIR_STORAGE_VOL_DISK_TYPE_PRIMARY; @@ -127,10 +122,29 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, vol->type = VIR_STORAGE_VOL_BLOCK; - /* The above gets allocation wrong for - * extended partitions, so overwrite it */ - vol->target.allocation = vol->target.capacity = - (vol->source.extents[0].end - vol->source.extents[0].start); + /* Refresh allocation/capacity/perms + * + * For an extended partition, virStorageBackendUpdateVolInfo will + * return incorrect values for allocation and capacity, so use the + * extent information captured above instead. + * + * Also once a logical partition exists or another primary partition + * after an extended partition is created an open on the extended + * partition will fail, so pass the NOERROR flag and only error if a + * -1 was returned indicating some other error than an open error. + */ + if (vol->source.partType == VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) { + if (virStorageBackendUpdateVolInfo(vol, true, false, + VIR_STORAGE_VOL_OPEN_DEFAULT | + VIR_STORAGE_VOL_OPEN_NOERROR) == -1) + return -1; + vol->target.allocation = vol->target.capacity = + (vol->source.extents[0].end - vol->source.extents[0].start); + } else { + if (virStorageBackendUpdateVolInfo(vol, true, false, + VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) + return -1; + } if (STRNEQ(groups[2], "metadata")) pool->def->allocation += vol->target.allocation; -- 2.1.0
When removing a volume that is the extended partition, all the logical volume partitions that exist within the extended partition will also be removed, so we need to refresh the pool to have the updated list Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_disk.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 233e293..300aab3 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -654,7 +654,7 @@ virStorageBackendDiskPartBoundaries(virStoragePoolObjPtr pool, static int -virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendDiskDeleteVol(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags) @@ -721,6 +721,15 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; } + /* If this was the extended partition, then all the logical partitions + * are then lost. Make it easy on ourselves and just refresh the pool + */ + if (vol->source.partType == VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) { + virStoragePoolObjClearVols(pool); + if (virStorageBackendDiskRefreshPool(conn, pool) < 0) + goto cleanup; + } + rc = 0; cleanup: VIR_FREE(devpath); -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1138516 If the provided volume name doesn't match what parted generated as the partition name, then return a failure. Update virsh.pod and formatstorage.html.in to describe the 'name' restriction for disk pools as well as the usage of the <target>'s <format type='value'>. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 12 ++++++++++-- src/storage/storage_backend_disk.c | 30 ++++++++++++++++++++++++------ tools/virsh.pod | 17 ++++++++++++++--- 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 933268c..d2e2bb8 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -463,7 +463,13 @@ <dl> <dt><code>name</code></dt> <dd>Providing a name for the volume which is unique to the pool. - This is mandatory when defining a volume. <span class="since">Since 0.4.1</span></dd> + This is mandatory when defining a volume. For a disk pool, the + name must be combination of the <code>source</code> device path + device and next partition number to be created. For example, if + the <code>source</code> device path is /dev/sdb and there are no + partitions on the disk, then the name must be sdb1 with the next + name being sdb2 and so on. + <span class="since">Since 0.4.1</span></dd> <dt><code>key</code></dt> <dd>Providing an identifier for the volume which identifies a single volume. In some cases it's possible to have two distinct keys @@ -553,7 +559,9 @@ <span class="since">Since 0.4.1</span></dd> <dt><code>format</code></dt> <dd>Provides information about the pool specific volume format. - For disk pools it will provide the partition type. For filesystem + For disk pools it will provide the partition table format type, but is + not preserved after a pool refresh or libvirtd restart. Use extended + in order to create an extended disk extent partition. For filesystem or directory pools it will provide the file format type, eg cow, qcow, vmdk, raw. If omitted when creating a volume, the pool's default format will be used. The actual format is specified via diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 300aab3..9f4d76a 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -47,16 +47,23 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, char **const groups, virStorageVolDefPtr vol) { - char *tmp, *devpath; + char *tmp, *devpath, *partname; + + /* Prepended path will be same for all partitions, so we can + * strip the path to form a reasonable pool-unique name + */ + if ((tmp = strrchr(groups[0], '/'))) + partname = tmp + 1; + else + partname = groups[0]; if (vol == NULL) { + /* This is typically a reload/restart/refresh path where + * we're discovering the existing partitions for the pool + */ if (VIR_ALLOC(vol) < 0) return -1; - /* Prepended path will be same for all partitions, so we can - * strip the path to form a reasonable pool-unique name - */ - tmp = strrchr(groups[0], '/'); - if (VIR_STRDUP(vol->name, tmp ? tmp + 1 : groups[0]) < 0 || + if (VIR_STRDUP(vol->name, partname) < 0 || VIR_APPEND_ELEMENT_COPY(pool->volumes.objs, pool->volumes.count, vol) < 0) { virStorageVolDefFree(vol); @@ -80,6 +87,17 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, return -1; } + /* Enforce provided vol->name is the same as what parted created. + * We do this after filling target.path so that we have a chance at + * deleting the partition with this failure from CreateVol path + */ + if (STRNEQ(vol->name, partname)) { + virReportError(VIR_ERR_INVALID_ARG, + _("invalid partition name '%s', expected '%s'"), + vol->name, partname); + return -1; + } + if (vol->key == NULL) { /* XXX base off a unique key of the underlying disk */ if (VIR_STRDUP(vol->key, vol->target.path) < 0) diff --git a/tools/virsh.pod b/tools/virsh.pod index abe80c2..1591341 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2983,7 +2983,11 @@ Create and start a pool object from the XML I<file>. Create and start a pool object I<name> from the raw parameters. If I<--print-xml> is specified, then print the XML of the pool object without creating the pool. Otherwise, the pool has the specified -I<type>. +I<type>. When using B<pool-create-as> for a pool of I<type> "disk", +the existing partitions found on the I<--source-dev path> will be used +to populate the disk pool. Therefore, it is suggested to use +B<pool-define-as> and B<pool-build> with the I<--overwrite> in order +to properly initialize the disk pool. [I<--source-host hostname>] provides the source hostname for pools backed by storage from a remote server (pool types netfs, iscsi, rbd, sheepdog, @@ -3175,13 +3179,20 @@ I<vol-name-or-key-or-path>] [I<--backing-vol-format> I<string>] Create a volume from a set of arguments. I<pool-or-uuid> is the name or UUID of the storage pool to create the volume in. -I<name> is the name of the new volume. +I<name> is the name of the new volume. For a disk pool, this must match the +partition name as determined from the pool's source device path and the next +available partition. For example, a source device path of /dev/sdb and there +are no partitions on the disk, then the name must be sdb1 with the next +name being sdb2 and so on. I<capacity> is the size of the volume to be created, as a scaled integer (see B<NOTES> above), defaulting to bytes if there is no suffix. I<--allocation> I<size> is the initial size to be allocated in the volume, also as a scaled integer defaulting to bytes. I<--format> I<string> is used in file based storage pools to specify the volume -file format to use; raw, bochs, qcow, qcow2, vmdk, qed. +file format to use; raw, bochs, qcow, qcow2, vmdk, qed. Use extended for disk +storage pools in order to create an extended partition (other values are +validity checked but not preserved when libvirtd is restarted or the pool +is refreshed). I<--backing-vol> I<vol-name-or-key-or-path> is the source backing volume to be used if taking a snapshot of an existing volume. I<--backing-vol-format> I<string> is the format of the snapshot backing volume; -- 2.1.0
On 22.01.2015 20:20, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1138516
v1: http://www.redhat.com/archives/libvir-list/2015-January/msg00465.html
In my previous attempt to resolve the issue, I created a stateDir and saved the volume XML for each pool in order to attempt to preserve the volume "name" and "target format type". However, it was pointed out during review that having a different "name" was not the original design intention. The intention was the name would be the partition name rather than something the user fabricates and expects to be saved/used. Since partition naming is handled by parted, the control over the name is not ours. Additionally, a pool refresh or libvirtd restart/reload would use the 'default' of the source device path and the partition number from the partition table.
The "simple" fix to the issue is in patch 6; however, as I was going through fixing this I realized a few things and found a few other issues along the way, namely:
Patches #1 & #2: After creating the partition if we fail something within the callback to virStorageBackendDiskMakeDataVol as a result of parsing the partition table from libvirt_parthelper, then the partition wasn't removed. So, patches 1 & 2 work at moving the DeleteVol code and then calling it if something in virStorageBackendDiskReadPartitions fails.
Patch #3: When determining what type of partitions currently exist in the pool we compared against the target.format which is supposed to be the file system format type of the partition on the target rather than whether the partition is a primary, extended, or logical partition on the source. Since we set the partType on the source while reading the partitions, that's what we should use.
Patch #4: During a refresh of the pool, we use virStorageBackendDiskReadPartitions in order to determine what types of partitions exist; however, if we have an extended partition and have created either a logical partition within or another primary partition after the extended one, then the open() will fail with ENXIO. So I special cased that.
Patch #5: When we delete an extended partition, any logical partitions that existed in the pool would still be listed even though as part of the delete partition logic all the logical partitions were also deleted.
Patch #6: So here is essentially the replacement for the previous patch series. Since the theory is one is supposed to know what they are doing and will provide the correct vol->name, make sure that they do so and cause a failure if they don't (indicating what the name should be). As an alternative we could just "overwrite" the name like we did anyway with pool refresh or libvirtd restart/reload by doing the following instead:
/* Like the reload/restart/refresh path - use the name created by * parted rather than the API/user provided name */ if (STRNEQ(vol->name, partname)) { VIR_FREE(vol->name); if (VIR_STRDUP(vol->name, partname) < 0) return -1; }
I also added details to the virsh.pod and formatstorage.html.in for the 'name' and the intersting "side effect" I found using 'virsh vol-create-as $pool $name $size --format extended'. I'd remove it, but I fear that someone else found this and has made use of it. The reality is that '--format' is supposed to be the file system format of the partition. However, the way it's used in the code will take what is supposed to be target format and allow creation of an extended partition. In virStorageBackendDiskPartFormat, if the pool source.format is DOS and the vol->target.format is VIR_STORAGE_VOL_DISK_EXTENDED, then we create an extended source partition as long as one doesn't already exist.
John Ferlan (6): storage: Move virStorageBackendDiskDeleteVol storage: Attempt error recovery in virStorageBackendDiskCreateVol storage: Fix check for partition type for disk backing volumes storage: Adjust how to refresh extended partition disk data storage: When delete extended partition, need to refresh pool storage: Check the partition name against provided name
docs/formatstorage.html.in | 12 +- src/storage/storage_backend.c | 4 + src/storage/storage_backend_disk.c | 235 +++++++++++++++++++++++-------------- tools/virsh.pod | 17 ++- 4 files changed, 174 insertions(+), 94 deletions(-)
ACK series Michal
participants (2)
-
John Ferlan -
Michal Privoznik