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@xxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list