From patchwork Mon Oct 21 22:25:19 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: NeilBrown X-Patchwork-Id: 3079921 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 44AF1BF924 for ; Mon, 21 Oct 2013 22:25:37 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 626FC20375 for ; Mon, 21 Oct 2013 22:25:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3746D20373 for ; Mon, 21 Oct 2013 22:25:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752107Ab3JUWZe (ORCPT ); Mon, 21 Oct 2013 18:25:34 -0400 Received: from cantor2.suse.de ([195.135.220.15]:48449 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751765Ab3JUWZd (ORCPT ); Mon, 21 Oct 2013 18:25:33 -0400 Received: from relay1.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id E4508A5743; Tue, 22 Oct 2013 00:25:31 +0200 (CEST) Date: Tue, 22 Oct 2013 09:25:19 +1100 From: NeilBrown To: Tony Asleson Cc: linux-nfs@vger.kernel.org, Steve Dickson Subject: Re: [PATCH] exportfs: Return non-zero exit value on error Message-ID: <20131022092519.4f4683a8@notabene.brown> In-Reply-To: <1380756584-13335-1-git-send-email-tasleson@redhat.com> References: <1380756584-13335-1-git-send-email-tasleson@redhat.com> X-Mailer: Claws Mail 3.9.0 (GTK+ 2.24.18; x86_64-suse-linux-gnu) Mime-Version: 1.0 Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Spam-Status: No, score=-7.3 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, 2 Oct 2013 18:29:44 -0500 Tony Asleson 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 > --- > 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 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 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':