[libvirt] [PATCH 2/4] storage: Refactor storage pool type checks

[Erik Skultety]

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