On 04/09/2014 08:42 PM, John Ferlan wrote: > Commit id '18642d10' caused a virt-test regression for NFS backend > storage error path checks when running the command: > > 'virsh find-storage-pool-sources-as netfs Unknown ' > > when the host did not have Gluster installed. Prior to the commit, > the test would fail with the error: > > error: internal error: Child process (/usr/sbin/showmount --no-headers > --exports Unknown) unexpected exit status 1: clnt_create: RPC: Unknown > host > > After the commit, the error would be ignored, the call would succeed, > and an empty list of pool sources returned. This was tucked into the > commit message as an expected outcome. > > When the target host does not have a GLUSTER_CLI this is a regression > over the previous release. Furthermore, even if Gluster CLI was present, > but had a failure to get devices, the API would return a failure even if > the NFS backend had found devices. > > Add code to handle the error conditions. > - If NFS fails > - If there is no Gluster CLI, then jump to cleanup/failure. > - If there is a Gluster CLI, then message the NFS > failure and continue with the Gluster checks. > > - If Gluster fails > - If NFS had failed, then jump to cleanup/failure. > - Message the Gluster failure and continue on. > > - If only one fails, fetch and return a list of source devices > even if it's empty > > Signed-off-by: John Ferlan <[email protected]> > --- > src/storage/storage_backend.c | 14 +++++++++++++ > src/storage/storage_backend.h | 1 + > src/storage/storage_backend_fs.c | 43 > +++++++++++++++++++++++++++++++++++----- > 3 files changed, 53 insertions(+), 5 deletions(-) > > diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c > index b56fefe..4d73902 100644 > --- a/src/storage/storage_backend.c > +++ b/src/storage/storage_backend.c > @@ -1643,6 +1643,13 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, > } > > #ifdef GLUSTER_CLI > +bool > +virStorageBackendHaveGlusterCLI(void) > +{ > + return true; > +} > + > + > int > virStorageBackendFindGlusterPoolSources(const char *host, > int pooltype, > @@ -1721,6 +1728,13 @@ virStorageBackendFindGlusterPoolSources(const char > *host, > return ret; > } > #else /* #ifdef GLUSTER_CLI */ > +bool > +virStorageBackendHaveGlusterCLI(void) > +{ > + return false; > +} > + > + > int > virStorageBackendFindGlusterPoolSources(const char *host ATTRIBUTE_UNUSED, > int pooltype ATTRIBUTE_UNUSED,
IMHO it's more readable to use GLUSTER_CLI directly, since we're not doing any
runtime detection.
> diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
> index 4f95000..41eed97 100644
> --- a/src/storage/storage_backend.h
> +++ b/src/storage/storage_backend.h
> @@ -87,6 +87,7 @@ int virStorageBackendFindFSImageTool(char **tool);
> virStorageBackendBuildVolFrom
> virStorageBackendFSImageToolTypeToFunc(int tool_type);
>
> +bool virStorageBackendHaveGlusterCLI(void);
> int virStorageBackendFindGlusterPoolSources(const char *host,
> int pooltype,
> virStoragePoolSourceListPtr
> list);
> diff --git a/src/storage/storage_backend_fs.c
> b/src/storage/storage_backend_fs.c
> index 4e4a7ae..f3d4a6d 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -238,9 +238,11 @@ virStorageBackendFileSystemNetFindPoolSourcesFunc(char
> **const groups,
> }
>
>
> -static void
> +static int
> virStorageBackendFileSystemNetFindNFSPoolSources(virNetfsDiscoverState
> *state)
> {
> + int ret = -1;
> +
> /*
> * # showmount --no-headers -e HOSTNAME
> * /tmp *
> @@ -267,9 +269,13 @@
> virStorageBackendFileSystemNetFindNFSPoolSources(virNetfsDiscoverState *state)
> if (virCommandRunRegex(cmd, 1, regexes, vars,
> virStorageBackendFileSystemNetFindPoolSourcesFunc,
> state, NULL) < 0)
> - virResetLastError();
> + goto cleanup;
>
> + ret = 0;
> +
> + cleanup:
> virCommandFree(cmd);
> + return ret;
> }
>
>
> @@ -289,6 +295,7 @@
> virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn
> ATTRIBUTE_UNUSE
> virStoragePoolSourcePtr source = NULL;
> char *ret = NULL;
> size_t i;
> + bool failNFS = false;
>
> virCheckFlags(0, NULL);
>
> @@ -310,12 +317,38 @@
> virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn
> ATTRIBUTE_UNUSE
>
> state.host = source->hosts[0].name;
>
> - virStorageBackendFileSystemNetFindNFSPoolSources(&state);
> + if (virStorageBackendFileSystemNetFindNFSPoolSources(&state) < 0) {
> + virErrorPtr err;
> + /* If no Gluster CLI, then force error right here */
> + if (!virStorageBackendHaveGlusterCLI())
> + goto cleanup;
> +
> + /* If we have a Gluster CLI, then message the error, clean it out,
> + * and move onto the Gluster code
> + */
> + err = virGetLastError();
> + VIR_ERROR(_("Failed to get NFS pool sources: '%s'"),
> + err != NULL ? err->message: _("unknown error"));
This takes the last logged error and logs it again with a prefix.
> + virResetLastError();
IMO we can leave the error set even if we return 0.
> + failNFS = true;
> + }
>
> if (virStorageBackendFindGlusterPoolSources(state.host,
>
> VIR_STORAGE_POOL_NETFS_GLUSTERFS,
> - &state.list) < 0)
> - goto cleanup;
> + &state.list) < 0) {
> + virErrorPtr err;
> + /* If NFS failed as well, then force the error right here */
> + if (failNFS)
> + goto cleanup;
> +
> + /* Otherwise, NFS passed, so we message the Gluster error, clean
> + * it out, and generate the source list (even if it's empty)
> + */
FindGlusterPoolSources doesn't report errors when the command returns non-zero.
> + err = virGetLastError();
> + VIR_ERROR(_("Failed to get Gluster pool sources: '%s'"),
> + err != NULL ? err->message: _("unknown error"));
> + virResetLastError();
> + }
>
The whole hunk would IMHO look simpler as:
bool fail = false;
if (FindNFSPoolSources() < 0)
fail = true;
#ifdef GLUSTER_CLI
if (FindGlusterPoolSources() < 0)
fail = true;
else
fail = false;
#endif
if (fail)
goto cleanup;
/* optionally */
else
virResetLastError();
(Though if we add more network backends, we would need a virBitmap to store
all the fails :-))
Jan
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
