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

[Daniel P. Berrange]

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 at 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
> > 
> 
> 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 :|