[libvirt] [PATCH] virsh: Check wether found volume is member of the specified storage pool
When looking up storage volumes virsh uses multiple lookup steps. Some of the steps don't require a pool name specified. This resulted into a possibility that a volume would be part of a different pool than the user specified: Let's have a /var/lib/libvirt/images/test.qcow image in the 'default' pool and a second pool 'emptypool': Currently we'd return: $ virsh vol-info --pool emptypool /var/lib/libvirt/images/test.qcow Name: test.qcow Type: file Capacity: 100.00 MiB Allocation: 212.00 KiB After the fix: $ tools/virsh vol-info --pool emptypool /var/lib/libvirt/images/test.qcow error: Requested volume '/var/lib/libvirt/images/test.qcow' found in a different pool (default) than specified Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1088667 --- tools/virsh-volume.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 55bf6f0..6416ba6 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -104,6 +104,26 @@ vshCommandOptVolBy(vshControl *ctl, const vshCmd *cmd, "might help"), n, pooloptname); } + /* If the pool was specified, then make sure that the returned + * volume is from the given pool */ + if (pool && vol) { + virStoragePoolPtr volpool = NULL; + + if ((volpool = virStoragePoolLookupByVolume(vol))) { + if (STRNEQ(virStoragePoolGetName(volpool), + virStoragePoolGetName(pool))) { + vshResetLibvirtError(); + vshError(ctl, + _("Requested volume '%s' found in a different " + "pool (%s) than specified"), + n, virStoragePoolGetName(volpool)); + virStorageVolFree(vol); + vol = NULL; + } + virStoragePoolFree(volpool); + } + } + if (pool) virStoragePoolFree(pool); -- 1.9.3
On Fri, May 30, 2014 at 03:39:36PM +0200, Peter Krempa wrote:
When looking up storage volumes virsh uses multiple lookup steps. Some of the steps don't require a pool name specified. This resulted into a possibility that a volume would be part of a different pool than the user specified:
Let's have a /var/lib/libvirt/images/test.qcow image in the 'default' pool and a second pool 'emptypool':
Currently we'd return: $ virsh vol-info --pool emptypool /var/lib/libvirt/images/test.qcow Name: test.qcow Type: file Capacity: 100.00 MiB Allocation: 212.00 KiB
I believed that the --pool parameter for vol-info (and *some* others) was only a hint in case you had more volumes with the same name (specifying absolute path with a pool doesn't make any sense). That would mean the BZ is notabug actually, but let's assume such users exist...
After the fix: $ tools/virsh vol-info --pool emptypool /var/lib/libvirt/images/test.qcow error: Requested volume '/var/lib/libvirt/images/test.qcow' found in a different pool (default) than specified
I'd say this is rather noisy. How about changing it to ...'%s' not found in pool '%s'... or "is not in pool '%s'? ACK after release with or without the change mentioned above. Martin
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1088667 --- tools/virsh-volume.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 55bf6f0..6416ba6 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -104,6 +104,26 @@ vshCommandOptVolBy(vshControl *ctl, const vshCmd *cmd, "might help"), n, portamento); }
+ /* If the pool was specified, then make sure that the returned + * volume is from the given pool */ + if (pool && vol) { + virStoragePoolPtr volpool = NULL; + + if ((volpool = virStoragePoolLookupByVolume(vol))) { + if (STRNEQ(virStoragePoolGetName(volpool), + virStoragePoolGetName(pool))) { + vshResetLibvirtError(); + vshError(ctl, + _("Requested volume '%s' found in a different " + "pool (%s) than specified"), + n, virStoragePoolGetName(volpool)); + virStorageVolFree(vol); + vol = NULL; + } + virStoragePoolFree(volpool); + } + } + if (pool) virStoragePoolFree(pool);
-- 1.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/30/14 16:28, Martin Kletzander wrote:
On Fri, May 30, 2014 at 03:39:36PM +0200, Peter Krempa wrote:
When looking up storage volumes virsh uses multiple lookup steps. Some of the steps don't require a pool name specified. This resulted into a possibility that a volume would be part of a different pool than the user specified:
Let's have a /var/lib/libvirt/images/test.qcow image in the 'default' pool and a second pool 'emptypool':
Currently we'd return: $ virsh vol-info --pool emptypool /var/lib/libvirt/images/test.qcow Name: test.qcow Type: file Capacity: 100.00 MiB Allocation: 212.00 KiB
I believed that the --pool parameter for vol-info (and *some* others) was only a hint in case you had more volumes with the same name (specifying absolute path with a pool doesn't make any sense). That would mean the BZ is notabug actually, but let's assume such users exist...
After the fix: $ tools/virsh vol-info --pool emptypool /var/lib/libvirt/images/test.qcow error: Requested volume '/var/lib/libvirt/images/test.qcow' found in a different pool (default) than specified
I'd say this is rather noisy. How about changing it to ...'%s' not found in pool '%s'... or "is not in pool '%s'?
ACK after release with or without the change mentioned above.
I went with your shorter wording and fixed the typo in the subject pointed out by Jan. Pushed now. Thanks. Peter
participants (3)
-
Ján Tomko -
Martin Kletzander -
Peter Krempa