[libvirt] [PATCH 5/5] storage: Add helper to compare logical pool def against pvs output
Michal Privoznik
mprivozn at redhat.com
Tue Dec 15 15:54:34 UTC 2015
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 at redhat.com>
> ---
> 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
More information about the libvir-list
mailing list