Re: [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(-)