[libvirt] [PATCH] storage: Better describe logical pool creation/definition parameters
Laine Stump
laine at laine.org
Mon Mar 27 17:51:02 UTC 2017
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 at 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
>>>
>>
>
More information about the libvir-list
mailing list