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

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(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 > 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.