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

[Martin Kletzander]

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.

Thanks,
Martin