[libvirt] [PATCH] Drop virStorageBackendLogicalMatchPoolSource

[John Ferlan]

On 06/15/2016 01:19 PM, Ján Tomko wrote:
> Regression introduced by commit 71b803a for [1] that prevents starting up
> a logical pool created with <source><device path=''></source>
> after it has been moved to a different physical volume.

Is there a bug for this?  XML examples?

Is an empty source device path string a valid value?  Reading
http://libvirt.org/formatstorage.html doesn't give me that impression.

"device
    Provides the source for pools backed by physical devices (pool types
fs, logical, disk, iscsi, zfs). May be repeated multiple times depending
on backend driver. Contains a required attribute path which is either
the fully qualified path to the block device node or for iscsi the iSCSI
Qualified Name (IQN). Since 0.4.1"


> 
> For logical pools <source><name> contains the name of the volume group
> and uniquely identifies the VG on the host.
> 
> This also speeds up startup for pools that do not have any <device>s
> specified.
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1025230
> ---
>  src/storage/storage_backend_logical.c | 104 +---------------------------------
>  1 file changed, 2 insertions(+), 102 deletions(-)
> 

This essentially reverts a patch that was used to resolve a bz without a
patch to resolve the issue in the bug.  What's the proposal/patch to
resolve the issue from the bug?


It would seem to me that if the "decision" was to accept an source
device path of '', then it's "simple enough" to add a check for that in
virStorageBackendLogicalMatchPoolSource before the memset()...

E.g.

    if (pool->def->source.name[0] == '\0')
        return true;

I think it's an oddball case, but I suppose valid.

John

> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
> index ca05fe1..057e0b9 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -558,107 +558,11 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED,
>  }
>  
>  
> -/*
> - * virStorageBackendLogicalMatchPoolSource
> - * @pool: Pointer to the source pool object
> - *
> - * Search the output generated by a 'pvs --noheadings -o pv_name,vg_name'
> - * to match the 'vg_name' with the pool def->source.name and for the list
> - * of pool def->source.devices[].
> - *
> - * Returns true if the volume group name matches the pool's source.name
> - * and at least one of the pool's def->source.devices[] matches the
> - * list of physical device names listed for the pool. Return false if
> - * we cannot find a matching volume group name and if we cannot match
> - * the any device list members.
> - */
> -static bool
> -virStorageBackendLogicalMatchPoolSource(virStoragePoolObjPtr pool)
> -{
> -    virStoragePoolSourceList sourceList;
> -    virStoragePoolSource *thisSource = NULL;
> -    size_t i, j;
> -    int matchcount = 0;
> -    bool ret = false;
> -
> -    memset(&sourceList, 0, sizeof(sourceList));
> -    sourceList.type = VIR_STORAGE_POOL_LOGICAL;
> -
> -    if (virStorageBackendLogicalGetPoolSources(&sourceList) < 0)
> -        goto cleanup;
> -
> -    /* Search the pvs output for this pool's source.name */
> -    for (i = 0; i < sourceList.nsources; i++) {
> -        thisSource = &sourceList.sources[i];
> -        if (STREQ(thisSource->name, pool->def->source.name))
> -            break;
> -    }
> -
> -    if (i == sourceList.nsources) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                       _("cannot find logical volume group name '%s'"),
> -                       pool->def->source.name);
> -        goto cleanup;
> -    }
> -
> -    /* If the pool has defined source device(s), then let's make sure
> -     * they match as well; otherwise, matching can only occur on the
> -     * pool's name.
> -     */
> -    if (!pool->def->source.ndevice) {
> -        ret = true;
> -        goto cleanup;
> -    }
> -
> -    /* Let's make sure the pool's device(s) match what the pvs output has
> -     * for volume group devices.
> -     */
> -    for (i = 0; i < pool->def->source.ndevice; i++) {
> -        for (j = 0; j < thisSource->ndevice; j++) {
> -            if (STREQ(pool->def->source.devices[i].path,
> -                      thisSource->devices[j].path))
> -                matchcount++;
> -        }
> -    }
> -
> -    /* If we didn't find any matches, then this pool has listed (a) source
> -     * device path(s) that don't/doesn't match what was created for the pool
> -     */
> -    if (matchcount == 0) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                       _("cannot find any matching source devices for logical "
> -                         "volume group '%s'"), pool->def->source.name);
> -        goto cleanup;
> -    }
> -
> -    /* Either there's more devices in the pool source device list or there's
> -     * more devices in the pvs output. Could easily happen if someone decides
> -     * to 'add' to or 'remove' from the volume group outside of libvirt's
> -     * knowledge. Rather than fail on that, provide a warning and move on.
> -     */
> -    if (matchcount != pool->def->source.ndevice)
> -        VIR_WARN("pool device list count doesn't match pvs device list count");
> -
> -    ret = true;
> -
> - cleanup:
> -    for (i = 0; i < sourceList.nsources; i++)
> -        virStoragePoolSourceClear(&sourceList.sources[i]);
> -    VIR_FREE(sourceList.sources);
> -
> -    return ret;
> -}
> -
> -
>  static int
>  virStorageBackendLogicalCheckPool(virStoragePoolObjPtr pool,
>                                    bool *isActive)
>  {
> -    /* If we can find the target.path as well as ensure that the
> -     * pool's def source
> -     */
> -    *isActive = virFileExists(pool->def->target.path) &&
> -                virStorageBackendLogicalMatchPoolSource(pool);
> +    *isActive = virFileExists(pool->def->target.path);
>      return 0;
>  }
>  
> @@ -666,11 +570,7 @@ static int
>  virStorageBackendLogicalStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                    virStoragePoolObjPtr pool)
>  {
> -    /* Let's make sure that the pool's name matches the pvs output and
> -     * that the pool's source devices match the pvs output.
> -     */
> -    if (!virStorageBackendLogicalMatchPoolSource(pool) ||
> -        virStorageBackendLogicalSetActive(pool, 1) < 0)
> +    if (virStorageBackendLogicalSetActive(pool, 1) < 0)
>          return -1;
>  
>      return 0;
>