src/conf/storage_conf.c | 11 +++++++++++ tools/virsh.pod | 15 +++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-)
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 >> > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 >>> >> > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.