On Mon, Jul 15, 2013 at 10:30:16AM +0200, Martin Kletzander wrote:
When using logical pools, we had to trust the target->path provided. This parameter, however, can be completely ommited and we can get the correct path using 'lvdisplay' command. In order not to omit the target.path completely, we rather default it to '/dev/<source.name>' which is used to check the pool anyway.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=952973 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: v3: - just rebase v2: - don't drop target.path, but fix it instead to '/dev/<source.name>'
Thanks to the change from v1, we can now safely drop all the hunks from logical backend and even the dependency on lvdisplay command. There might be some systems where the path is different and the part of this patch using lvdisplay command would make most of them work. However, checkPool() still depends on '/dev/<source.name>' and I haven't found any other way how to fix that, so feel free to ACK just the {conf,docs,tests}/ part in case you think that we shouldn't try to support anything else than '/dev/<source.name>'.
configure.ac | 4 + docs/schemas/storagepool.rng | 13 +++- src/conf/storage_conf.c | 19 +++-- src/storage/storage_backend_logical.c | 88 ++++++++++++++-------- src/storage/storage_driver.c | 2 +- tests/storagepoolxml2xmlin/pool-logical-create.xml | 2 +- tests/storagepoolxml2xmlin/pool-logical-nopath.xml | 18 +++++ .../storagepoolxml2xmlout/pool-logical-nopath.xml | 19 +++++ tests/storagepoolxml2xmltest.c | 1 + 9 files changed, 125 insertions(+), 41 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-logical-nopath.xml create mode 100644 tests/storagepoolxml2xmlout/pool-logical-nopath.xml
diff --git a/configure.ac b/configure.ac index b5af0d3..967e70a 100644 --- a/configure.ac +++ b/configure.ac @@ -1601,6 +1601,7 @@ if test "$with_storage_lvm" = "yes" || test "$with_storage_lvm" = "check"; then AC_PATH_PROG([PVS], [pvs], [], [$PATH:/sbin:/usr/sbin]) AC_PATH_PROG([VGS], [vgs], [], [$PATH:/sbin:/usr/sbin]) AC_PATH_PROG([LVS], [lvs], [], [$PATH:/sbin:/usr/sbin]) + AC_PATH_PROG([LVDISPLAY], [lvdisplay], [], [$PATH:/sbin:/usr/sbin])
[snip]
+ + virCommandPtr cmd = virCommandNewArgList(LVDISPLAY, + "--columns", + "--options", "lv_path", + "--noheadings", + "--unbuffered", + NULL);
Why are you introducing use of 'lvdisplay', when our existing code uses 'lvs' ? 'lvs' can tell you everything that 'lvdisplay' can and handles the case we have here # lvdisplay --columns --options lv_path --noheadings --unbuffered vg_t500wlan/lv_root /dev/vg_t500wlan/lv_root # lvs --options lv_path --noheadings --unbuffered vg_t500wlan/lv_root /dev/vg_t500wlan/lv_root Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|