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

[Martin Kletzander]

On 06/03/2013 06:22 PM, Osier Yang wrote:
> On 03/06/13 23:05, Martin Kletzander wrote:
>> When using logical pools, we had to trust the target->pth provided.
> 
> s/pth/path/,
> 
>> This parameter, however, can be completely ommited and we can get the
>> correct path using 'lvdisplay' command.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=952973
>>
>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> ---
>>
>> Notes:
>>      I'm not sure we can drop the target.path from the XML (see
>> tests), so
>>      another variant is to keep it there the same way it was defined by
>>      user/mgmt app.
> How about one file a bug "The created volume is not under specified pool's
> target path"? Since with this patch, the specified pool's target path is
> silently
> ignored.
> 
> If you have a good answer/reason/fix for above question, I see no
> problem of
> ignore the pool->target.path, but the path you got from lvdisplay (with
> vol name
> is truncated) should be reflected to the pool's XML.
> 

Sorry for such late reply, but I didn't get to this issue until now.

The problem about updating the target.path is that the function that deals with the pool creation gets different copy of data that the one that builds the pool.  I already forgot why I haven't rewritten the whole mess in a way that allows the changing of parameters even in the building function, so I'll dig into that and maybe seeing the problem after a while will help and I'll get this fixed.

>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index cc3d3d9..948e190 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -1,7 +1,7 @@
>>   /*
>>    * storage_conf.c: config handling for storage driver
>>    *
>> - * Copyright (C) 2006-2012 Red Hat, Inc.
>> + * Copyright (C) 2006-2013 Red Hat, Inc.
>>    * Copyright (C) 2006-2008 Daniel P. Berrange
>>    *
>>    * This library is free software; you can redistribute it and/or
>> @@ -947,15 +947,16 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
>>       /* When we are working with a virtual disk we can skip the target
>>        * path and permissions */
>>       if (!(options->flags & VIR_STORAGE_POOL_SOURCE_NETWORK)) {
>> -        if (!(target_path = virXPathString("string(./target/path)",
>> ctxt))) {
>> -            virReportError(VIR_ERR_XML_ERROR, "%s",
>> -                           _("missing storage pool target path"));
>> -            goto error;
>> +        if (ret->type != VIR_STORAGE_POOL_LOGICAL) {
>> +            if (!(target_path =
>> virXPathString("string(./target/path)", ctxt))) {
>> +                virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                               _("missing storage pool target path"));
>> +                goto error;
>> +            }
>> +            ret->target.path = virFileSanitizePath(target_path);
>> +            if (!ret->target.path)
>> +                goto error;
> 
> RNG schema needs to be changed too..
> 

You mean making the target.path optional?  I think it is already.  Or did you mean something else?

[...]
>> @@ -314,7 +340,7 @@
>> virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
>>                                  "--unbuffered",
>>                                  "--nosuffix",
>>                                  "--options",
>> -                              
>> "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_attr",
>>
> 
> I won't want to see what I added (lv_attr) is removed by mistake, :-)
> 

I have no idea how that happened.  I probably missed that it is used to skip inactive volumes.  Will fix that

[...]
>> diff --git a/tests/storagepoolxml2xmlin/pool-logical-create.xml
>> b/tests/storagepoolxml2xmlin/pool-logical-create.xml
>> index 4c67089..e1bb4db 100644
>> --- a/tests/storagepoolxml2xmlin/pool-logical-create.xml
>> +++ b/tests/storagepoolxml2xmlin/pool-logical-create.xml
>> @@ -10,7 +10,6 @@
>>       <device path="/dev/sdb3"/>
>>     </source>
>>     <target>
>> -    <path>/dev/HostVG</path>
> 
> At least one test should have the specified target path, to make sure
> the old XML still work..
> 

Good point, but I'll modify the tests accordingly to the change anyway ;-)

Martin