[libvirt] [PATCH v2 3/4] storage: Better describe logical pool creation/definition parameters

[John Ferlan]

On 03/27/2017 05:19 PM, Jiri Denemark wrote:
> On Mon, Mar 27, 2017 at 13:42:22 -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1398087
>>
>> Clean up the virsh man page description for --pool-create-as in order
>> to better describe how the various arguments are used when creating
>> (or defining) a logical pool.
>>
>> Also modify the storage pool XML parsing algorithm to check for the
>> mismatched "name" and "source-name".
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/conf/storage_conf.c | 8 ++++++++
>>  tools/virsh.pod         | 7 +++++++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 585ca71..5213503 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -760,6 +760,14 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
>>              if (VIR_STRDUP(ret->source.name, ret->name) < 0)
>>                  goto error;
>>          }
>> +        if (ret->type == VIR_STORAGE_POOL_LOGICAL &&
>> +            STRNEQ(ret->name, ret->source.name)) {
>> +                virReportError(VIR_ERR_XML_ERROR,
>> +                               _("for a logical pool, the pool name='%s' "
>> +                                 "must match the pool source name='%s'"),
>> +                               ret->name, ret->source.name);
>> +                goto error;
> 
> Wrong indentation...
> 
>> +        }
> 
> but why exactly is this forbidden now? I should be able to create a pool
> with a (libvirt's) name which differs from the (system's) name of the
> volume group, shouldn't I? And apparently it used to work while it is
> not working now after this patch as failing virt-manager builds on
> ci.centos.org suggest.
> 


If 'source.name' isn't supplied it defaults to ret->name.  The ret->name
is supposed to be the Volume Group name (it's the "unique" to the host
name). Although I suppose it could be something different, but if only
the name is required in order to define/create the pool, then how does
one "ensure" that the name provided is the volume group name. It's kind
of a conundrum... Still I certainly suppose someone could do something
different, so I suppose this part could be reverted.

FWIW: The 'source.name' is used is by the logical backend for various
vg* and lv* commands. So if it's not the Volume Group name, then
commands fail (as seen in the bz).

I really think it's just "smart sense" to make them be the same to avoid
"confusion".

John