Re: [libvirt] [PATCH v2 3/4] storage: Better describe logical pool creation/definition parameters

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(a)redhat.com&gt; >> --- >> 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.

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".