[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