[libvirt] [PATCHv2 0/6] Resolve issues in disk pool backend

[Michal Privoznik]

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