[libvirt] [PATCHv2 0/6] Resolve issues in disk pool backend
Michal Privoznik
mprivozn at redhat.com
Wed Jan 28 15:46:05 UTC 2015
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
More information about the libvir-list
mailing list