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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list