On Mon, Jul 15, 2013 at 02:39:28PM +0200, Martin Kletzander wrote: > On 07/15/2013 02:27 PM, Daniel P. Berrange wrote: > > 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@xxxxxxxxxx> > >> --- > >> > >> 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 > > > > I guess I haven't managed to make some older (or buggy) version to > display that info. That makes it so much easier then, updated version > works ok, in case anyone has a problem, we'll see then, but that > shouldn't happen. > > Before posting v2, what's your opinion on supporting only > '/dev/<source.name>' as mentioned in notes above? It would drop most of > the code. I think relying on /dev/GROUPNAME/VOLNAME as the path is fine. 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list