Re: [PATCH 5/5] storage: Add helper to compare logical pool def against pvs output

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 07.12.2015 21:47, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1025230
> 
> Add a new helper virStorageBackendLogicalMatchPoolSource to compare the
> pool's source name against the output from a 'pvs' command to list all
> volume group physical volume data on the host.  In addition, compare the
> pool's source device list against the particular volume group's device
> list to ensure the source device(s) listed for the pool match what the
> was listed for the volume group.
> 
> Then for pool startup or check API's we need to call this new API in
> order to ensure that the pool we're about to start or declare active
> during checkPool has a valid definition vs. the running host.
> 
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
>  src/storage/storage_backend_logical.c | 96 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 2 deletions(-)
> 
> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
> index 53ba983..6dea9d2 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -498,11 +498,99 @@ 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;
> +    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;
> +    }
> +
> +    /* 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)
>  {
> -    *isActive = virFileExists(pool->def->target.path);
> +    /* If we can find the target.path as well as ensure that the
> +     * pool's def source
> +     */
> +    if (virFileExists(pool->def->target.path) &&
> +        virStorageBackendLogicalMatchPoolSource(pool))
> +        *isActive = true;

missing 'else *isActive = false;';

or just use *isActive = virFileExists() &&
virStorageBackendLogicalMatchPoolSource();

We should not rely on chance that caller sets isActive = false prior
calling this function.

>      return 0;
>  }
>  
> @@ -510,7 +598,11 @@ static int
>  virStorageBackendLogicalStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                    virStoragePoolObjPtr pool)
>  {
> -    if (virStorageBackendLogicalSetActive(pool, 1) < 0)
> +    /* 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)
>          return -1;
>  
>      return 0;
> 

ACK with that small nit fixed.

Michal

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