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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list