On 06/03/2015 01:44 PM, John Ferlan wrote:

Over time the parameters changed, but the comment wasn't updated

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

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 337b8d3..b70902a 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -336,7 +336,6 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE

/** - * @conn connection to report errors against * @pool storage pool to check for status * * Determine if a storage pool is already mounted @@ -369,7 +368,6 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) }

/** - * @conn connection to report errors against * @pool storage pool to mount * * Ensure that a FS storage pool is mounted on its target location. @@ -474,7 +472,6 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) }

/** - * @conn connection to report errors against * @pool storage pool to unmount * * Ensure that a FS storage pool is not mounted on its target location.

ACK, I'll have a look at the rest of the series later today. Erik

On 06/03/2015 01:44 PM, John Ferlan wrote:

Refactor the code for both startPool (*Mount) and stopPool (*Unmount) code paths by introducing virStorageBackendFileSystemValidateFS.

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

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index b70902a..9a1343d 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -334,6 +334,41 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE return ret; }

+/** + * @pool storage pool to check FS types + * + * Determine if storage pool FS types are properly set up + * + * Return 0 if everything's OK, -1 on error + */ +static int +virStorageBackendFileSystemValidateFS(virStoragePoolObjPtr pool) +{ Hmm, I see the word 'filesystem' twice in the function name (though one of them is abbreviation only - doesn't matter), how about 'virStorageBackendFileSystemIsValid' or something like that... + if (pool->def->type == VIR_STORAGE_POOL_NETFS) { + if (pool->def->source.nhost != 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Expected exactly 1 host for the storage pool")); ^^ I know this is a historical issue, but since you're refactoring, how about converting the error message to lowercase to stay error-consistent. + return -1; + } + if (pool->def->source.hosts[0].name == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing source host")); + return -1; + } + if (pool->def->source.dir == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing source path")); + return -1; + } + } else { + if (pool->def->source.ndevice != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing source device")); + return -1; + } + } + return 0; +}

/** * @pool storage pool to check for status @@ -390,29 +425,8 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) int ret = -1; int rc;

- if (pool->def->type == VIR_STORAGE_POOL_NETFS) { - if (pool->def->source.nhost != 1) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Expected exactly 1 host for the storage pool")); - return -1; - } - if (pool->def->source.hosts[0].name == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing source host")); - return -1; - } - if (pool->def->source.dir == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing source path")); - return -1; - } - } else { - if (pool->def->source.ndevice != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing source device")); - return -1; - } - } + if (virStorageBackendFileSystemValidateFS(pool) < 0) + return -1;

/* Short-circuit if already mounted */ if ((rc = virStorageBackendFileSystemIsMounted(pool)) != 0) { @@ -486,29 +500,8 @@ virStorageBackendFileSystemUnmount(virStoragePoolObjPtr pool) int ret = -1; int rc;

- if (pool->def->type == VIR_STORAGE_POOL_NETFS) { - if (pool->def->source.nhost != 1) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Expected exactly 1 host for the storage pool")); - return -1; - } - if (pool->def->source.hosts[0].name == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing source host")); - return -1; - } - if (pool->def->source.dir == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing source dir")); - return -1; - } - } else { - if (pool->def->source.ndevice != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing source device")); - return -1; - } - } + if (virStorageBackendFileSystemValidateFS(pool) < 0) + return -1;

/* Short-circuit if already unmounted */ if ((rc = virStorageBackendFileSystemIsMounted(pool)) != 1)

Otherwise it looks fine, so ACK with those little nitpicks. Erik

On 06/03/2015 01:44 PM, John Ferlan wrote:

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

Currently the assumption on the error message is that there are no source device path's defined when the != 1 check fails, but in s/path's/paths

"...when the != 1 check fails" I can't help it, I know what you're saying but somehow I keep thinking about it in C logic --> when a check fails, it means that the structured block following the check will be skipped...anyway, it's nothing, maybe I'm just thinking about it too much, this one's your call.

reality the value could 0 or 2 or more, so adjust the error message accordingly to make it clearer what the error really is.

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

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 9a1343d..3c39646 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -362,8 +362,13 @@ virStorageBackendFileSystemValidateFS(virStoragePoolObjPtr pool) } } else { if (pool->def->source.ndevice != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing source device")); + if (pool->def->source.ndevice == 0) + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing source device")); + else + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", I'd say this should be CONFIG_UNSUPPORTED rather than INTERNAL_ERROR + _("Expected exactly 1 device for the " ^ lowercase again + "storage pool")); return -1; } }

ACK with those minor adjustments (as I said, the one in the commit message is up to you).

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

The virStorageBackendFileSystemIsMounted is called from three source paths checkPool, startPool, and stopPool. Both start and stop validate the FS fields before calling *IsMounted; however the check path there is no call. I'd adjust this "however the check path there is no call" to something

On 06/03/2015 01:44 PM, John Ferlan wrote: like this "however there is no FS validation call in the check path"

This could lead the code into returning a true in "isActive" if for some reason the target path for the pool was mounted. The assumption being that if it was mounted, then we believe we started/mounted it.

It's also of note that commit id '81165294' added an error message for the start/mount path regarding that the target is already mounted so fail the start. That check was adjusted by commit id '13fde7ce' to only message if actually mounted.

At one time this led to the libvirtd restart autostart code to declare that the pool was active even though the startPool would inhibit startup and the stopPool would inhibit shutdown. The autostart path changed as of commit id '2a31c5f0' as part of the keep storage pools started between libvirtd restarts.

This patch adds the same check made prior to start/mount and stop/unmount to ensure we have a valid configuration before attempting to see if the target is already mounted to declare "isActive" or not. Finding an improper configuration will now cause an error at checkPool, which should make it so we can no longer be left in a situation where the pool was started and we have no way to stop it.

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

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 3c39646..b8c5a59 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -537,6 +537,10 @@ virStorageBackendFileSystemCheck(virStoragePoolObjPtr pool, } else { int ret; *isActive = false; + + if (virStorageBackendFileSystemValidateFS(pool) < 0) + return -1; + if ((ret = virStorageBackendFileSystemIsMounted(pool)) != 0) { if (ret < 0) return -1;

ACK. Erik