Re: [libvirt] [PATCH v3] Make logical pools independent on target path

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(a)redhat.com&gt; > --- > > 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