[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]