On 03/27/2017 11:37 AM, John Ferlan wrote:
On 03/26/2017 08:38 PM, Laine Stump wrote:
> On 03/25/2017 08:18 AM, 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 move the --print-xml to the end of the qualifiers since it's not
>> properly positionally situated for both --pool-create-as and --pool-define-as.
>>
>> Finally modify the storage pool XML parsing algorithm to check for the
>> mismatched "name" and "source-name" as well as a more general
if not
>> provided, then set the default source format.
>
> I think the change to storage_conf.c should be separate from the virsh
> manpage fix.
>
Do you mean both storage_conf adjustments? I could see separating out
the formatFromString, but the STRNEQ is related to the issue from the bz
that someone could provide a different name for both and that's related
to the new description in the virsh.pod file.
I could make 3 patches out of this, but it just didn't feel "necessary"
(the 3rd being movement of the --print-xml argument in the virsh.pod
file). But if that's what's desired, fine...
Maybe it's the ordering of the items in the commit log that throws me
off. For an outsider, it sounds like you made a couple of changes to the
docs, one to more properly reflect current behavior, then another to
just reformat, and then also made a change in the source code to change
"something" - just reading the commit log it seems like they're unrelated.
But I guess what you're saying is that the source has been changed to
alter behavior slightly, and you updated the info in virsh.pod to
reflect that (while also making a formatting tweak). If that's the case,
then it's okay - maybe just describe the source code change first and
then the doc change as it relates to the source change.
For that I can ACK.
John
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/conf/storage_conf.c | 11 +++++++++++
>> tools/virsh.pod | 15 +++++++++++----
>> 2 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 6b34cea..6ca4949 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -703,6 +703,9 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
>> if (virStoragePoolDefParseSource(ctxt, &ret->source,
ret->type,
>> source_node) < 0)
>> goto error;
>> + } else {
>> + if (options->formatFromString)
>> + ret->source.format = options->defaultFormat;
>> }
>>
>> ret->name = virXPathString("string(./name)", ctxt);
>> @@ -757,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;
>> + }
>> }
>>
>> if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) &&
>> diff --git a/tools/virsh.pod b/tools/virsh.pod
>> index ee79046..a1b4086 100644
>> --- a/tools/virsh.pod
>> +++ b/tools/virsh.pod
>> @@ -3572,13 +3572,13 @@ follow-up command to build the pool. The
I<--overwrite> and
>> I<--no-overwrite> flags follow the same rules as B<pool-build>. If
>> just I<--build> is provided, then B<pool-build> is called with no
flags.
>>
>> -=item B<pool-create-as> I<name> I<type>
[I<--print-xml>]
>> +=item B<pool-create-as> I<name> I<type>
>> [I<--source-host hostname>] [I<--source-path path>]
[I<--source-dev path>]
>> [I<--source-name name>] [I<--target path>] [I<--source-format
format>]
>> [I<--auth-type authtype> I<--auth-username username>
I<--secret-usage usage>]
>> [[I<--adapter-name name>] | [I<--adapter-wwnn>
I<--adapter-wwpn>]
>> [I<--adapter-parent parent>]]
>> -[I<--build>] [[I<--overwrite>] | [I<--no-overwrite>]]
>> +[I<--build>] [[I<--overwrite>] | [I<--no-overwrite>]]
[I<--print-xml>]
>>
>>
>> Create and start a pool object I<name> from the raw parameters. If
>> @@ -3628,17 +3628,24 @@ follow-up command to build the pool. The
I<--overwrite> and
>> I<--no-overwrite> flags follow the same rules as B<pool-build>. If
>> just I<--build> is provided, then B<pool-build> is called with no
flags.
>>
>> +For a "logical" pool only [I<--name>] needs to be provided. The
[I<--name>]
>> +must match the Volume Group name for which the pool is being defined or
>> +created. The [I<--source-name>] if provided must match the Volume Group
>> +name. If not provided, one will be generated using the [I<--name>]. If
>> +provided the [I<--target>] is ignored and a target source is generated
>> +using the [I<--source-name>] (or as generated from the
[I<--name>]).
>> +
>> =item B<pool-define> I<file>
>>
>> Define an inactive persistent storage pool or modify an existing persistent one
>> from the XML I<file>.
>>
>> -=item B<pool-define-as> I<name> I<type>
[I<--print-xml>]
>> +=item B<pool-define-as> I<name> I<type>
>> [I<--source-host hostname>] [I<--source-path path>]
[I<--source-dev path>]
>> [I<--source-name name>] [I<--target path>] [I<--source-format
format>]
>> [I<--auth-type authtype> I<--auth-username username>
I<--secret-usage usage>]
>> [[I<--adapter-name name>] | [I<--adapter-wwnn>
I<--adapter-wwpn>]
>> -[I<--adapter-parent parent>]]
>> +[I<--adapter-parent parent>]] [I<--print-xml>]
>>
>> Create, but do not start, a pool object I<name> from the raw parameters.
If
>> I<--print-xml> is specified, then print the XML of the pool object
>>
>