[libvirt] [PATCH] storage: Better describe logical pool creation/definition parameters
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. Signed-off-by: John Ferlan <jferlan@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 -- 2.9.3
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.
Signed-off-by: John Ferlan <jferlan@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
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... John
Signed-off-by: John Ferlan <jferlan@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
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@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
On Sat, Mar 25, 2017 at 08:18:46AM -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 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.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 11 +++++++++++ tools/virsh.pod | 15 +++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-)
@@ -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);
Why? Jan
participants (3)
-
John Ferlan -
Ján Tomko -
Laine Stump