On 07.12.2015 21:47, John Ferlan wrote:

Refactor the code that builds the pool source string during the FS storage pool mount to be a separate helper.

A future patch will use the helper in order to validate the mounted FS matches the pool's expectation during poolCheck processing

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 51 +++++++++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 99ea394..ef1a7d0 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -375,6 +375,39 @@ virStorageBackendFileSystemIsValid(virStoragePoolObjPtr pool) return 0; }

+ +/** + * virStorageBackendFileSystemGetPoolSource + * @pool: storage pool object pointer + * + * Allocate/return a string representing the FS storage pool source. + * It is up to the caller to VIR_FREE the allocated string + */ +static char * +virStorageBackendFileSystemGetPoolSource(virStoragePoolObjPtr pool) +{ + char *src = NULL; + + if (pool->def->type == VIR_STORAGE_POOL_NETFS) { + if (pool->def->source.format == VIR_STORAGE_POOL_NETFS_CIFS) { + if (virAsprintf(&src, "//%s/%s", + pool->def->source.hosts[0].name, + pool->def->source.dir) == -1) + return NULL;

When touching this, I'd s/== -1/< 0/. Then, these if()-s seem a bit useless - if virAsprintf() fails, @src is going to be NULL anyway. Your call if you want to do ignore_value(virAsprintf(...));

+ } else { + if (virAsprintf(&src, "%s:%s", + pool->def->source.hosts[0].name, + pool->def->source.dir) == -1) + return NULL; + } + } else { + if (VIR_STRDUP(src, pool->def->source.devices[0].path) < 0) + return NULL; + } + return src; +} + + /** * @pool storage pool to check for status * @@ -445,22 +478,8 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) return -1; }

- if (pool->def->type == VIR_STORAGE_POOL_NETFS) { - if (pool->def->source.format == VIR_STORAGE_POOL_NETFS_CIFS) { - if (virAsprintf(&src, "//%s/%s", - pool->def->source.hosts[0].name, - pool->def->source.dir) == -1) - return -1; - } else { - if (virAsprintf(&src, "%s:%s", - pool->def->source.hosts[0].name, - pool->def->source.dir) == -1) - return -1; - } - } else { - if (VIR_STRDUP(src, pool->def->source.devices[0].path) < 0) - return -1; - } + if (!(src = virStorageBackendFileSystemGetPoolSource(pool))) + return -1;

if (netauto) cmd = virCommandNewArgList(MOUNT,

ACK Michal

On 07.12.2015 21:47, John Ferlan wrote:

Refactor code to use standard return functioning with respect to setting a ret value and going to cleanup.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index ef1a7d0..1dd5727 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -418,6 +418,7 @@ virStorageBackendFileSystemGetPoolSource(virStoragePoolObjPtr pool) static int virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) { + int ret = -1; FILE *mtab; struct mntent ent; char buf[1024]; @@ -426,18 +427,21 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) virReportSystemError(errno, _("cannot read mount list '%s'"), _PATH_MOUNTED); - return -1; + goto cleanup; }

while ((getmntent_r(mtab, &ent, buf, sizeof(buf))) != NULL) { if (STREQ(ent.mnt_dir, pool->def->target.path)) { - VIR_FORCE_FCLOSE(mtab); - return 1; + ret = 1; + goto cleanup; } }

+ ret = 0; + + cleanup: VIR_FORCE_FCLOSE(mtab); - return 0; + return ret; }

/**

ACK Michal

On 07.12.2015 21:47, John Ferlan wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1025230

When determining whether a FS pool is mounted, rather than assuming that the FS pool is mounted just because the target.path is in the mount list, let's make sure that the FS pool source matches what is mounted

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 1dd5727..70e6be6 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -419,6 +419,7 @@ static int virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) { int ret = -1; + char *src = NULL; FILE *mtab; struct mntent ent; char buf[1024]; @@ -431,16 +432,23 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) }

while ((getmntent_r(mtab, &ent, buf, sizeof(buf))) != NULL) { - if (STREQ(ent.mnt_dir, pool->def->target.path)) { + if (!(src = virStorageBackendFileSystemGetPoolSource(pool))) + goto cleanup; + + if (STREQ(ent.mnt_dir, pool->def->target.path) && + STREQ(ent.mnt_fsname, src)) { ret = 1; goto cleanup; } + + VIR_FREE(src); }

ret = 0;

cleanup: VIR_FORCE_FCLOSE(mtab); + VIR_FREE(src); return ret; }

ACK Michal

On 07.12.2015 21:47, John Ferlan wrote:

Rework virStorageBackendLogicalFindPoolSources a bit to create a helper virStorageBackendLogicalGetPoolSources that will make the pvs call in order to generate a list of associated pv_name and vg_name's.

A future patch will make use of this for start/check processing to ensure the storage pool source definition matches expectations.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 52 ++++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 536e617..53ba983 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -414,10 +414,16 @@ virStorageBackendLogicalFindPoolSourcesFunc(char **const groups, return -1; }

-static char * -virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, - const char *srcSpec ATTRIBUTE_UNUSED, - unsigned int flags) +/* + * @sourceList: Pointer to a storage pool source list + * + * Use the pvs command to fill the list of pv_name and vg_name associated + * into the passed sourceList. + * + * Returns 0 if successful, -1 and sets error on failure + */ +static int +virStorageBackendLogicalGetPoolSources(virStoragePoolSourceListPtr sourceList) { /* * # pvs --noheadings -o pv_name,vg_name @@ -431,11 +437,7 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, 2 }; virCommandPtr cmd; - char *retval = NULL; - virStoragePoolSourceList sourceList; - size_t i; - - virCheckFlags(0, NULL); + int ret = -1;

/* * NOTE: ignoring errors here; this is just to "touch" any logical volumes @@ -447,20 +449,38 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, VIR_WARN("Failure when running vgscan to refresh physical volumes"); virCommandFree(cmd);

- memset(&sourceList, 0, sizeof(sourceList)); - sourceList.type = VIR_STORAGE_POOL_LOGICAL; - cmd = virCommandNewArgList(PVS, "--noheadings", "-o", "pv_name,vg_name", NULL); if (virCommandRunRegex(cmd, 1, regexes, vars, virStorageBackendLogicalFindPoolSourcesFunc, - &sourceList, "pvs") < 0) { - virCommandFree(cmd); - return NULL; - } + sourceList, "pvs") < 0) + goto cleanup; + ret = 0; + + cleanup: virCommandFree(cmd); + return ret; +} + + +static char * +virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, + const char *srcSpec ATTRIBUTE_UNUSED, + unsigned int flags) +{ + virStoragePoolSourceList sourceList; + size_t i; + char *retval = NULL; + + virCheckFlags(0, NULL); + + memset(&sourceList, 0, sizeof(sourceList)); + sourceList.type = VIR_STORAGE_POOL_LOGICAL; + + if (virStorageBackendLogicalGetPoolSources(&sourceList) < 0) + goto cleanup;

retval = virStoragePoolSourceListFormat(&sourceList); if (retval == NULL) {

ACK Michal

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@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