[libvirt] [PATCH v2 0/4] storage: Better describe logical pool creation/definition parameters
v1: https://www.redhat.com/archives/libvir-list/2017-March/msg01227.html changes since v1: Split things part - added news article John Ferlan (4): conf: Set defaultFormat if no storage source XML present virsh.pod: Move the positional --print-xml for pool-{define|create}-as storage: Better describe logical pool creation/definition parameters docs: Add news article for logical pool-create-as docs/news.xml | 5 +++++ src/conf/storage_conf.c | 11 +++++++++++ tools/virsh.pod | 15 +++++++++++---- 3 files changed, 27 insertions(+), 4 deletions(-) -- 2.9.3
While parsing if the storage source is not present, then a defaultFormat was not set. This could lead to oddities such as seeing "unknown" format in output for the "logical" pool even though the only format the pool could support would be "lvm2". This does "put a label" on other pool defaults as follows: File System: FS_AUTO Network File System: NETFS_AUTO Disk: UNKNOWN Each of which is the "0" value for their respective pools and thus would be no "real" change. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 28277e5..585ca71 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); -- 2.9.3
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 and could be miscontrued as being the 3rd positional argument. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh.pod | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index 7fa7985..43124ba 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3583,13 +3583,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 @@ -3644,12 +3644,12 @@ just I<--build> is provided, then B<pool-build> is called with no flags. 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
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@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; + } } if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) && diff --git a/tools/virsh.pod b/tools/virsh.pod index 43124ba..55b71a9 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3639,6 +3639,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. +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 -- 2.9.3
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@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. Jirka
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@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
On Mon, Mar 27, 2017 at 20:03:42 -0400, John Ferlan wrote:
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@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.
Sure, if only one name is provided, it's OK to use it in both places (i.e., pool name volume group name).
Still I certainly suppose someone could do something different, so I suppose this part could be reverted.
Definitely, it has to be reverted. Otherwise existing pools which don't follow this logic will just disappear when libvirtd starts again.
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).
Indeed, source.name has to be to volume group name.
I really think it's just "smart sense" to make them be the same to avoid "confusion".
Perhaps, but it doesn't mean we should forbid pools with different names. And especially when such pools used to work just fine. Jirka
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/news.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index a772799..29b3546 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -700,6 +700,11 @@ logical: Need to overwrite/clear more than just first 512 bytes </summary> </change> + <change> + <summary> + Describe the logical backend requirements better for pool-create-as + </summary> + </change> </section> </release> </libvirt> -- 2.9.3
On Mon, Mar 27, 2017 at 01:42:23PM -0400, John Ferlan wrote:
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/news.xml | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/docs/news.xml b/docs/news.xml index a772799..29b3546 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -700,6 +700,11 @@ logical: Need to overwrite/clear more than just first 512 bytes </summary> </change> + <change> + <summary> + Describe the logical backend requirements better for pool-create-as
Does this really add more value than noise to release notes for the people reading it?
+ </summary> + </change> </section> </release> </libvirt> -- 2.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 03/27/2017 04:35 PM, Martin Kletzander wrote:
On Mon, Mar 27, 2017 at 01:42:23PM -0400, John Ferlan wrote:
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/news.xml | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/docs/news.xml b/docs/news.xml index a772799..29b3546 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -700,6 +700,11 @@ logical: Need to overwrite/clear more than just first 512 bytes </summary> </change> + <change> + <summary> + Describe the logical backend requirements better for pool-create-as
Does this really add more value than noise to release notes for the people reading it?
Geez, I'd figure a fix related to a bugzilla filed by some libvirt consumer could qualify to be something we note as being changed. If you feel otherwise, then I'm not against reverting. I did what I thought was right. John
+ </summary> + </change> </section> </release> </libvirt> -- 2.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 03/27/2017 01:42 PM, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2017-March/msg01227.html
changes since v1:
Split things part - added news article
John Ferlan (4): conf: Set defaultFormat if no storage source XML present virsh.pod: Move the positional --print-xml for pool-{define|create}-as storage: Better describe logical pool creation/definition parameters docs: Add news article for logical pool-create-as
docs/news.xml | 5 +++++ src/conf/storage_conf.c | 11 +++++++++++ tools/virsh.pod | 15 +++++++++++---- 3 files changed, 27 insertions(+), 4 deletions(-)
Okay, ACK series. Sorry you had to go through all the rearranging just because I didn't understand the relationship between the bits.
participants (4)
-
Jiri Denemark -
John Ferlan -
Laine Stump -
Martin Kletzander