Re: [PATCH] exportfs: Return non-zero exit value on error

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 8772 bytes --]

On Wed,  2 Oct 2013 18:29:44 -0500 Tony Asleson <tasleson@redhat.com> wrote:

> To improve error handling when scripting exportfs it's useful
> to have non-zero exit codes when the requested operation did not
> succeed.
> 
> This patch also returns a non-zero exit code if you request to
> unexport a non-existant share.
> 
> Signed-off-by: Tony Asleson <tasleson@redhat.com>
> ---
>  support/export/hostname.c |  2 ++
>  utils/exportfs/exportfs.c | 37 +++++++++++++++++++++++++++----------
>  2 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/support/export/hostname.c b/support/export/hostname.c
> index 3e949a1..e53d692 100644
> --- a/support/export/hostname.c
> +++ b/support/export/hostname.c
> @@ -175,10 +175,12 @@ host_addrinfo(const char *hostname)
>  	case 0:
>  		return ai;
>  	case EAI_SYSTEM:
> +		export_errno = errno;
>  		xlog(D_GENERAL, "%s: failed to resolve %s: (%d) %m",
>  				__func__, hostname, errno);
>  		break;
>  	default:
> +		export_errno = EINVAL;
>  		xlog(D_GENERAL, "%s: failed to resolve %s: %s",
>  				__func__, hostname, gai_strerror(error));
>  		break;
> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
> index 52fc03d..318deb3 100644
> --- a/utils/exportfs/exportfs.c
> +++ b/utils/exportfs/exportfs.c
> @@ -35,8 +35,8 @@
>  #include "xlog.h"
>  
>  static void	export_all(int verbose);
> -static void	exportfs(char *arg, char *options, int verbose);
> -static void	unexportfs(char *arg, int verbose);
> +static int	exportfs(char *arg, char *options, int verbose);
> +static int	unexportfs(char *arg, int verbose);
>  static void	exports_update(int verbose);
>  static void	dump(int verbose, int export_format);
>  static void	error(nfs_export *exp, int err);
> @@ -187,8 +187,12 @@ main(int argc, char **argv)
>  		if (f_all)
>  			export_all(f_verbose);
>  		else
> -			for (i = optind; i < argc ; i++)
> -				exportfs(argv[i], options, f_verbose);
> +			for (i = optind; i < argc ; i++) {
> +				if(!exportfs(argv[i], options, f_verbose)) {
> +					/* Only flag a generic EINVAL if no errno is set */
> +					export_errno = (export_errno) ? export_errno : EINVAL;
> +				}
> +			}
>  	}
>  	/* If we are unexporting everything, then
>  	 * don't care about what should be exported, as that
> @@ -201,8 +205,12 @@ main(int argc, char **argv)
>  		if (!f_reexport)
>  			xtab_export_read();
>  		if (!f_export)
> -			for (i = optind ; i < argc ; i++)
> -				unexportfs(argv[i], f_verbose);
> +			for (i = optind ; i < argc ; i++) {
> +				if (!unexportfs(argv[i], f_verbose)) {
> +					/* Only flag a generic EINVAL if no errno is set */
> +					export_errno = (export_errno) ? export_errno : EINVAL;
> +				}
> +			}
>  		if (!new_cache)
>  			rmtab_read();
>  	}
> @@ -296,9 +304,10 @@ export_all(int verbose)
>  }
>  
>  
> -static void
> +static int
>  exportfs(char *arg, char *options, int verbose)
>  {
> +	int rc = 0;
>  	struct exportent *eep;
>  	nfs_export	*exp = NULL;
>  	struct addrinfo	*ai = NULL;
> @@ -311,7 +320,8 @@ exportfs(char *arg, char *options, int verbose)
>  
>  	if (!path || *path != '/') {
>  		xlog(L_ERROR, "Invalid exporting option: %s", arg);
> -		return;
> +		export_errno = EINVAL;
> +		return rc;
>  	}
>  
>  	if ((htype = client_gettype(hname)) == MCL_FQDN) {
> @@ -339,13 +349,16 @@ exportfs(char *arg, char *options, int verbose)
>  	exp->m_warned = 0;
>  	validate_export(exp);
>  
> +	rc = 1;
>  out:
>  	freeaddrinfo(ai);
> +	return rc;
>  }
>  
> -static void
> +static int
>  unexportfs(char *arg, int verbose)
>  {
> +	int rc = 0;
>  	nfs_export	*exp;
>  	struct addrinfo *ai = NULL;
>  	char		*path;
> @@ -357,7 +370,8 @@ unexportfs(char *arg, int verbose)
>  
>  	if (!path || *path != '/') {
>  		xlog(L_ERROR, "Invalid unexporting option: %s", arg);
> -		return;
> +		export_errno = EINVAL;
> +		return rc;
>  	}
>  
>  	if ((htype = client_gettype(hname)) == MCL_FQDN) {
> @@ -397,9 +411,11 @@ unexportfs(char *arg, int verbose)
>  #endif
>  		exp->m_xtabent = 0;
>  		exp->m_mayexport = 0;
> +		rc = 1;
>  	}
>  
>  	freeaddrinfo(ai);
> +	return rc;
>  }
>  
>  static int can_test(void)
> @@ -728,6 +744,7 @@ error(nfs_export *exp, int err)
>  {
>  	xlog(L_ERROR, "%s:%s: %s", exp->m_client->m_hostname,
>  		exp->m_export.e_path, strerror(err));
> +	export_errno = (export_errno) ? export_errno : err;
>  }
>  
>  static void

This seems the have been forgotten, so maybe by replying to it someone will
notice (hi Steve).

Though I agree with the need for the patch, I don't much like it's shape.

Why change exportfs and unexportfs to return a status?  The status is only
used to set export_errno, and they sometimes set export_errno anyway, so why
not leave them returning  void and just setting export_errno as needed?


My preference is actually to get 'xlog' to set export_errno.

See below.

Thanks,
NeilBrown



From 2f63d6c3c38e673216a3351aff47d32059056638 Mon Sep 17 00:00:00 2001
From: Neil Brown <neilb@suse.de>
Date: Mon, 21 Oct 2013 17:40:55 +1100
Subject: [PATCH] exportfs: exit with error code if there was any error.

exportfs currently exits with a non-zero error for some errors,
but not for others.

It does this by having various support routines set the global
variable "export_errno".

Change this to have 'xlog' set export_errno if an ERROR is
reported.  That way all errors will be caught.

Note that the exit error code is changed from 22 (EINVAL)
to the more traditional '1'.

Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/support/include/exportfs.h b/support/include/exportfs.h
index 1fbf754..97b2327 100644
--- a/support/include/exportfs.h
+++ b/support/include/exportfs.h
@@ -179,7 +179,4 @@ struct export_features {
 struct export_features *get_export_features(void);
 void fix_pseudoflavor_flags(struct exportent *ep);
 
-/* Record export error.  */
-extern int export_errno;
-
 #endif /* EXPORTFS_H */
diff --git a/support/include/xlog.h b/support/include/xlog.h
index fd1a3f4..fd34ec2 100644
--- a/support/include/xlog.h
+++ b/support/include/xlog.h
@@ -35,6 +35,7 @@ struct xlog_debugfac {
 	int		df_fac;
 };
 
+extern int export_errno;
 void			xlog_open(char *progname);
 void			xlog_stderr(int on);
 void			xlog_syslog(int on);
diff --git a/support/nfs/exports.c b/support/nfs/exports.c
index d3160d3..d18667f 100644
--- a/support/nfs/exports.c
+++ b/support/nfs/exports.c
@@ -47,8 +47,6 @@ struct flav_info flav_map[] = {
 
 const int flav_map_size = sizeof(flav_map)/sizeof(flav_map[0]);
 
-int export_errno;
-
 static char	*efname = NULL;
 static XFILE	*efp = NULL;
 static int	first;
@@ -133,7 +131,6 @@ getexportent(int fromkernel, int fromexports)
 	}
 	if (ok < 0) {
 		xlog(L_ERROR, "expected client(options...)");
-		export_errno = EINVAL;
 		return NULL;
 	}
 	first = 0;
@@ -153,7 +150,6 @@ getexportent(int fromkernel, int fromexports)
 		ok = getexport(exp, sizeof(exp));
 		if (ok < 0) {
 			xlog(L_ERROR, "expected client(options...)");
-			export_errno = EINVAL;
 			return NULL;
 		}
 	}
@@ -173,7 +169,6 @@ getexportent(int fromkernel, int fromexports)
 		*opt++ = '\0';
 		if (!(sp = strchr(opt, ')')) || sp[1] != '\0') {
 			syntaxerr("bad option list");
-			export_errno = EINVAL;
 			return NULL;
 		}
 		*sp = '\0';
@@ -590,7 +585,6 @@ parseopts(char *cp, struct exportent *ep, int warn, int *had_subtree_opt_ptr)
 				     flname, flline, opt);	
 bad_option:
 				free(opt);
-				export_errno = EINVAL;
 				return -1;
 			}
 		} else if (strncmp(opt, "anongid=", 8) == 0) {
diff --git a/support/nfs/xlog.c b/support/nfs/xlog.c
index 6820346..9f9e63e 100644
--- a/support/nfs/xlog.c
+++ b/support/nfs/xlog.c
@@ -38,6 +38,8 @@ static int  logmask = 0;		/* What will be logged		*/
 static char log_name[256];		/* name of this program		*/
 static int  log_pid = -1;		/* PID of this program		*/
 
+int export_errno = 0;
+
 static void	xlog_toggle(int sig);
 static struct xlog_debugfac	debugnames[] = {
 	{ "general",	D_GENERAL, },
@@ -189,6 +191,8 @@ void
 xlog(int kind, const char* fmt, ...)
 {
 	va_list args;
+	if (kind & (L_ERROR|D_GENERAL))
+		export_errno = 1;
 
 	va_start(args, fmt);
 	xlog_backend(kind, fmt, args);
diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
index 4331697..c2cb2fc 100644
--- a/utils/exportfs/exportfs.c
+++ b/utils/exportfs/exportfs.c
@@ -103,8 +103,6 @@ main(int argc, char **argv)
 	xlog_stderr(1);
 	xlog_syslog(0);
 
-	export_errno = 0;
-
 	while ((c = getopt(argc, argv, "afhio:ruv")) != EOF) {
 		switch(c) {
 		case 'a':

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]