On Wed, Feb 13, 2013 at 02:03:16PM +0100, Ulrich Windl wrote:
> Hi!
>
> I've made a patch to let exportfs propagate the errors it reported to the
> exit code of the process (see attachments, the compressed tar is there in
> case the mailer corrupts the patche files):
>
> The exportfs seems to have been written with a "hot needle", not having error
> processing in mind. So I have to change the prototypes of several routines.
> Also one routine returns 1 on success, and 0 of failure, which is against all
> C conventions. Anyway, I've tested my code a littel bit, and it looks
> promising:
>
> # non-root calling exportfs
> ./exportfs; echo $?
> exportfs: could not open /var/lib/nfs/.etab.lock for locking: errno 13
> (Permission denied)
> 1
> # Empty export list
> # ./exportfs; echo $?
> 0
> # ./exportfs no-such-host:/tmp; echo $?
> exportfs: Failed to resolve no-such-host
> 1
> # existing host and filesystem
> rksapv04:/tmp # ./exportfs localhost:/tmp; echo $?
> 0
> # non-existing filesystem or directory
> # ./exportfs localhost:/no-such-file; echo $?
> exportfs: Failed to stat /no-such-file: No such file or directory
> exportfs: localhost:/no-such-file: No such file or directory
> 1
> # both incorrect
> # ./exportfs no-host:/no-file; echo $?
> exportfs: Failed to resolve no-host
> exportfs: localhost:/no-such-file: No such file or directory
> 1
> # unfortunately the non-existing filesystem is still exported:
> # ./exportfs ; echo $?
> /tmp localhost
> /no-such-file localhost
> 0
> # At least it can be unexported
> # ./exportfs -u localhost:/no-such-file; echo $?
> 0
>
> So the patch is not perfect, but probably a good staring point. Please send
> feedback!
Thanks, agreed that this is worth fixing.
I'm used to the kernel convention:
int try_something(void)
{
return -ERRNO;
}
ret = try_something();
if (ret)
return ret;
...
so this convention is a bit alien to me, but I suppose it's fine as long
as we aren't losing any error information that the callers could use.
Cc'ing steved as this is an nfs-utils patch.
--b.
>
> Regards,
> Ulrich
>
> >>> Ulrich Windl schrieb am 01.02.2013 um 09:47 in Nachricht <510B8119.CA2 :
> >>> 161 :
> 60728>:
> > >>> Dejan Muhamedagic <[email protected]> schrieb am 01.02.2013 um 08:53
> > >>> in
> > Nachricht <[email protected]>:
> > > Hi,
> > >
> > > On Fri, Feb 01, 2013 at 08:24:46AM +0100, Ulrich Windl wrote:
> > > > Hi!
> > > >
> > > > While trying to develop an improved exportfs RA that can export one
> > > filesystem to a list of names (instead of just one name), I found an
> > > error:
> > > >
> > > > exportfs is returning exit code 0 even if the filesystem could not be
> > > exported, like in
> > > >
> > > > > h02:~ # exportfs h012:/mnt; echo $? # h012 does not exist
> > > > > exportfs: Failed to resolve h012
> > > > > 0
> > > >
> > > > Accordingly the start operation for the exportfs alway reports success,
> > > even if the operation failed! That's due to
> > > >
> > > > [...]
> > > > > ocf_run exportfs -v ${OPTIONS}
> > > ${OCF_RESKEY_clientspec}:${OCF_RESKEY_directory} || exit $OCF_ERR_GENERIC
> > >
> > >
> > > > >
> > > > > # Restore the rmtab to ensure smooth NFS-over-TCP failover
> > > > > restore_rmtab
> > > > >
> > > > > ocf_log info "File system exported"
> > > > > return $OCF_SUCCESS
> > > > [...]
> > > >
> > > > However the monitor operation does the correct thing, thus reporting an
> > > error (rc==7).
> > >
> > > We could insert the monitor operation after the call to exportfs.
> > > Would there be any timing issues? I guess not.
> > >
> > > Thanks,
> > >
> > > Dejan
> >
> > Hi!
> >
> > I'd fix the root of the problem: exportfs is unreliable!
> >
> > Regards,
> > Ulrich
> >
> >
> >
> > >
> > > > This combination leads to the message of ocf-tester:
> > > > * rc=1: Monitoring an active resource should return 0
> > > >
> > > > (The tester thinks the resource was started when it was not)
> > > >
> > > > I had reported this for SLES10 SP2 to support in November, but after a
> > > > long
> > > time of waiting and repeating the same facts over and over, support told
> > > me
> > > it's "working as designed"; lvscan and lvcreate would be similar.
> > > >
> > > > A quick test showed that this is actually not true vor lvcreate:
> > > > # lvcreate -n bar -L40G foobar; echo $?
> > > > Volume group "foobar" not found
> > > > 5
> > > > # lvcreate -n foo -L1T sys; echo $?
> > > > Volume group "sys" has insufficient free space (1855 extents): 32768
> > > required.
> > > > 5
> > > >
> > > > I got the impression that my support is just unwilling to fix the
> > > > rather
> > > trivial problem. My exportfs comes from nfs-kernel-server-1.2.3-18.23.1.
> > > >
> > > > Getting the exportfs sources, it took me two minutes to find out that
> > > exportfs uses variable export_errno to define the exit code, and that
> > > variable is nowhere set. The worker routine exportfs() does return
> > > nothing
> > at
> > > all. What a design!
> > > >
> > > > Here's an example from an old HP-UX system that didn't get any updates
> > > > for
> > > three years:
> > > >
> > > > # exportfs nohost:/mnt; echo $?
> > > > exportfs: no entry for nohost:/mnt in /etc/dfs/dfstab
> > > > 1
> > > >
> > > > Here it works!
> > > >
> > > > I wonder why some Linux people are so ignorant occasionally...
> > > >
> > > > Regards,
> > > > Ulrich
> > > >
> > > >
> > > > _______________________________________________
> > > > Linux-HA mailing list
> > > > [email protected]
> > > > http://lists.linux-ha.org/mailman/listinfo/linux-ha
> > > > See also: http://linux-ha.org/ReportingProblems
> > > _______________________________________________
> > > Linux-HA mailing list
> > > [email protected]
> > > http://lists.linux-ha.org/mailman/listinfo/linux-ha
> > > See also: http://linux-ha.org/ReportingProblems
> > >
> >
> >
> >
> >
>
>
>
> >From 11c4a0d45a06169f2de8bd220509f4569a5c37c2 Mon Sep 17 00:00:00 2001
> From: Ulrich Windl <[email protected]>
> Date: Wed, 13 Feb 2013 11:45:33 +0100
> Subject: [PATCH 1/2] Include <stdio.h> for snprintf()
>
> Include <stdio.h> to declare snprintf() in utils/nfsdcltrack/sqlite.c.
> ---
> utils/nfsdcltrack/sqlite.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/utils/nfsdcltrack/sqlite.c b/utils/nfsdcltrack/sqlite.c
> index bac6789..f825873 100644
> --- a/utils/nfsdcltrack/sqlite.c
> +++ b/utils/nfsdcltrack/sqlite.c
> @@ -38,6 +38,7 @@
> #include "config.h"
> #endif /* HAVE_CONFIG_H */
>
> +#include <stdio.h>
> #include <dirent.h>
> #include <errno.h>
> #include <event.h>
> --
> 1.7.12.4
>
> >From eaaa966427e64b247f67a22df3ed87fe6dd61923 Mon Sep 17 00:00:00 2001
> From: Ulrich Windl <[email protected]>
> Date: Wed, 13 Feb 2013 13:24:54 +0100
> Subject: [PATCH 2/2] Let exportfs exit with error on errors
>
> support/export/xtab.c:
> xtab_read() and xtab_export_read() will return 0 on errors (strange logic,
> but according to existing code).
>
> support/include/nfslib.h, support/nfs/cacheio.c:
> cache_flush() will return 0 on success.
>
> utils/exportfs/exportfs.c:
> Changed export_all(), exportfs(), unexportfs(), exports_update(),
> validate_export(), grab_lockfile(), release_lockfile() to return 0 on
> success. Added verbose_release_lockfile() to keep atexit() happy.
> Changed main() to return 1 on failure of any of: cache_flush(),
> xtab_export_read(), grab_lockfile(), export_all(), exportfs(),
> unexportfs(), exports_update(), xtab_export_write(), xtab_mount_write()
> Changed exports_update_one() to return 0 on success, watching
> export_export() and export_unexport() for errors.
> Changed exports_update() to return 0 on success, watching
> exports_update_one() for errors.
> Changed export_all() to return 0 on success watching validate_export() for
> errors.
> Changed exportfs() to return 0 on success, watching updateexportent() and
> validate_export() for errors.
> Changed validate_export() to return 0 on success, watching test_export()
> for errors.
> ---
> support/export/xtab.c | 4 +-
> support/include/nfslib.h | 2 +-
> support/nfs/cacheio.c | 5 +-
> utils/exportfs/exportfs.c | 173
> ++++++++++++++++++++++++++++++----------------
> 4 files changed, 120 insertions(+), 64 deletions(-)
>
> diff --git a/support/export/xtab.c b/support/export/xtab.c
> index 2a43193..a87d453 100644
> --- a/support/export/xtab.c
> +++ b/support/export/xtab.c
> @@ -24,6 +24,7 @@
> int v4root_needed;
> static void cond_rename(char *newfile, char *oldfile);
>
> +/* return 0 on failure! */
> static int
> xtab_read(char *xtab, char *lockfn, int is_export)
> {
> @@ -63,7 +64,7 @@ xtab_read(char *xtab, char *lockfn, int is_export)
> endexportent();
> xfunlock(lockid);
>
> - return 0;
> + return 1;
> }
>
> int
> @@ -93,6 +94,7 @@ xtab_export_read(void)
> * inode number changes when the xtab_export_write is done. If you change the
> * routine below such that the files are edited in place, then you'll need to
> * fix the auth_reload logic as well...
> + * Return 0 on failure!
> */
> static int
> xtab_write(char *xtab, char *xtabtmp, char *lockfn, int is_export)
> diff --git a/support/include/nfslib.h b/support/include/nfslib.h
> index f210a06..ee45f56 100644
> --- a/support/include/nfslib.h
> +++ b/support/include/nfslib.h
> @@ -155,7 +155,7 @@ int qword_eol(FILE *f);
> int readline(int fd, char **buf, int *lenp);
> int qword_get(char **bpp, char *dest, int bufsize);
> int qword_get_int(char **bpp, int *anint);
> -void cache_flush(int force);
> +int cache_flush(int force);
> int check_new_cache(void);
> void qword_add(char **bpp, int *lp, char *str);
> void qword_addhex(char **bpp, int *lp, char *buf, int blen);
> diff --git a/support/nfs/cacheio.c b/support/nfs/cacheio.c
> index e641c45..7ffca97 100644
> --- a/support/nfs/cacheio.c
> +++ b/support/nfs/cacheio.c
> @@ -321,7 +321,7 @@ check_new_cache(void)
> * auth.unix.ip nfsd.export nfsd.fh
> */
>
> -void
> +int
> cache_flush(int force)
> {
> struct stat stb;
> @@ -329,6 +329,7 @@ cache_flush(int force)
> char stime[20];
> char path[200];
> time_t now;
> + int result = 0;
> /* Note: the order of these caches is important.
> * They need to be flushed in dependancy order. So
> * a cache that references items in another cache,
> @@ -357,8 +358,10 @@ cache_flush(int force)
> if (write(fd, stime, strlen(stime)) !=
> (ssize_t)strlen(stime)) {
> xlog_warn("Writing to '%s' failed: errno %d
> (%s)",
> path, errno, strerror(errno));
> + result = 1;
> }
> close(fd);
> }
> }
> + return result;
> }
> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
> index 9f79541..27ac8c3 100644
> --- a/utils/exportfs/exportfs.c
> +++ b/utils/exportfs/exportfs.c
> @@ -34,18 +34,19 @@
> #include "exportfs.h"
> #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 void exports_update(int verbose);
> +static int export_all(int verbose);
> +static int exportfs(char *arg, char *options, int verbose);
> +static int unexportfs(char *arg, int verbose);
> +static int exports_update(int verbose);
> static void dump(int verbose);
> static void error(nfs_export *exp, int err);
> static void usage(const char *progname, int n);
> -static void validate_export(nfs_export *exp);
> +static int validate_export(nfs_export *exp);
> static int matchhostname(const char *hostname1, const char *hostname2);
> static void export_d_read(const char *dname);
> -static void grab_lockfile(void);
> -static void release_lockfile(void);
> +static int grab_lockfile(void);
> +static int release_lockfile(void);
> +static void verbose_release_lockfile(void);
>
> static const char *lockfile = EXP_LOCKFILE;
> static int _lockfd = -1;
> @@ -66,18 +67,28 @@ static int _lockfd = -1;
> * corrupting etab, but to prevent problems like the above we
> * need these additional lockfile() routines.
> */
> -static void
> +static int
> grab_lockfile()
> {
> _lockfd = open(lockfile, O_CREAT|O_RDWR, 0666);
> - if (_lockfd != -1)
> - lockf(_lockfd, F_LOCK, 0);
> + if (_lockfd != -1 && lockf(_lockfd, F_LOCK, 0) == 0)
> + return 0;
> + return 1;
> }
> -static void
> +
> +static int
> release_lockfile()
> {
> - if (_lockfd != -1)
> - lockf(_lockfd, F_ULOCK, 0);
> + if (_lockfd != -1 && lockf(_lockfd, F_ULOCK, 0) == 0)
> + return 0;
> + return 1;
> +}
> +
> +static void
> +verbose_release_lockfile()
> +{
> + if (release_lockfile())
> + xlog(L_ERROR, "problem releasing lockfile %s: %m", lockfile);
> }
>
> int
> @@ -153,38 +164,44 @@ main(int argc, char **argv)
> new_cache = check_new_cache();
> if (optind == argc && ! f_all) {
> if (force_flush) {
> - if (new_cache)
> - cache_flush(1);
> - else {
> + if (new_cache) {
> + if (cache_flush(1))
> + export_errno = 1;
> + } else {
> xlog(L_ERROR, "-f is available only "
> "with new cache controls. "
> "Mount /proc/fs/nfsd first");
> return 1;
> }
> - return 0;
> + return export_errno;
> } else {
> - xtab_export_read();
> + if (xtab_export_read() == 0)
> + export_errno = 1;
> dump(f_verbose);
> - return 0;
> + return export_errno;
> }
> }
>
> /*
> * Serialize things as best we can
> */
> - grab_lockfile();
> - atexit(release_lockfile);
> + if (grab_lockfile())
> + export_errno = 1;
> + atexit(verbose_release_lockfile);
>
> if (f_export && ! f_ignore) {
> export_read(_PATH_EXPORTS);
> export_d_read(_PATH_EXPORTS_D);
> }
> if (f_export) {
> - if (f_all)
> - export_all(f_verbose);
> - else
> - for (i = optind; i < argc ; i++)
> - exportfs(argv[i], options, f_verbose);
> + if (f_all) {
> + if (export_all(f_verbose))
> + export_errno = 1;
> + } else
> + for (i = optind; i < argc ; i++) {
> + if (exportfs(argv[i], options, f_verbose))
> + export_errno = 1;
> + }
> }
> /* If we are unexporting everything, then
> * don't care about what should be exported, as that
> @@ -194,30 +211,41 @@ main(int argc, char **argv)
> /* note: xtab_*_read does not update entries if they already
> exist,
> * so this will not lose new options
> */
> - if (!f_reexport)
> - xtab_export_read();
> + if (!f_reexport) {
> + if (xtab_export_read() == 0)
> + export_errno = 1;
> + }
> 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))
> + export_errno = 1;
> + }
> if (!new_cache)
> rmtab_read();
> }
> if (!new_cache) {
> xtab_mount_read();
> - exports_update(f_verbose);
> + if (exports_update(f_verbose))
> + export_errno = 1;
> + }
> + if (!xtab_export_write())
> + export_errno = 1;
> + if (new_cache) {
> + if (cache_flush(force_flush))
> + export_errno = 1;
> }
> - xtab_export_write();
> - if (new_cache)
> - cache_flush(force_flush);
> if (!new_cache)
> - xtab_mount_write();
> + if (!xtab_mount_write())
> + export_errno = 1;
>
> return export_errno;
> }
>
> -static void
> +static int
> exports_update_one(nfs_export *exp, int verbose)
> {
> + int result = 0;
> +
> /* check mountpoint option */
> if (exp->m_mayexport &&
> exp->m_export.e_mountpoint &&
> @@ -227,6 +255,7 @@ exports_update_one(nfs_export *exp, int verbose)
> printf("%s not exported as %s not a mountpoint.\n",
> exp->m_export.e_path, exp->m_export.e_mountpoint);
> exp->m_mayexport = 0;
> + result = 1;
> }
> if (exp->m_mayexport && ((exp->m_exported<1) || exp->m_changed)) {
> if (verbose)
> @@ -234,17 +263,22 @@ exports_update_one(nfs_export *exp, int verbose)
> exp->m_exported ?"re":"",
> exp->m_client->m_hostname,
> exp->m_export.e_path);
> - if (!export_export(exp))
> + if (!export_export(exp)) {
> error(exp, errno);
> + result = 1;
> + }
> }
> if (exp->m_exported && ! exp->m_mayexport) {
> if (verbose)
> printf("unexporting %s:%s from kernel\n",
> exp->m_client->m_hostname,
> exp->m_export.e_path);
> - if (!export_unexport(exp))
> + if (!export_unexport(exp)) {
> error(exp, errno);
> + result = 1;
> + }
> }
> + return result;
> }
>
>
> @@ -253,28 +287,33 @@ exports_update_one(nfs_export *exp, int verbose)
> * entries with m_exported but not m_mayexport get unexported
> * looking at m_client->m_type == MCL_FQDN and m_client->m_type == MCL_GSS
> only
> */
> -static void
> +static int
> exports_update(int verbose)
> {
> nfs_export *exp;
> + int result = 0;
>
> for (exp = exportlist[MCL_FQDN].p_head; exp; exp=exp->m_next) {
> - exports_update_one(exp, verbose);
> + if (exports_update_one(exp, verbose))
> + result = 1;
> }
> for (exp = exportlist[MCL_GSS].p_head; exp; exp=exp->m_next) {
> - exports_update_one(exp, verbose);
> + if (exports_update_one(exp, verbose))
> + result = 1;
> }
> + return result;
> }
>
> /*
> - * export_all finds all entries and
> - * marks them xtabent and mayexport so that they get exported
> + * export_all finds all entries and marks them xtabent and mayexport so that
> + * they get exported. Return 0 if thewre were no errors.
> */
> -static void
> +static int
> export_all(int verbose)
> {
> nfs_export *exp;
> int i;
> + int result = 0;
>
> for (i = 0; i < MCL_MAXTYPES; i++) {
> for (exp = exportlist[i].p_head; exp; exp = exp->m_next) {
> @@ -286,13 +325,15 @@ export_all(int verbose)
> exp->m_mayexport = 1;
> exp->m_changed = 1;
> exp->m_warned = 0;
> - validate_export(exp);
> + if (validate_export(exp))
> + result = 1;
> }
> }
> + return result;
> }
>
> -
> -static void
> +/* return 0 on success */
> +static int
> exportfs(char *arg, char *options, int verbose)
> {
> struct exportent *eep;
> @@ -301,13 +342,14 @@ exportfs(char *arg, char *options, int verbose)
> char *path;
> char *hname = arg;
> int htype;
> + int result = 0;
>
> if ((path = strchr(arg, ':')) != NULL)
> *path++ = '\0';
>
> if (!path || *path != '/') {
> xlog(L_ERROR, "Invalid exporting option: %s", arg);
> - return;
> + return 1;
> }
>
> if ((htype = client_gettype(hname)) == MCL_FQDN) {
> @@ -321,10 +363,14 @@ exportfs(char *arg, char *options, int verbose)
>
> if (!exp) {
> if (!(eep = mkexportent(hname, path, options)) ||
> - !(exp = export_create(eep, 0)))
> + !(exp = export_create(eep, 0))) {
> + result = 1;
> goto out;
> - } else if (!updateexportent(&exp->m_export, options))
> + }
> + } else if (!updateexportent(&exp->m_export, options)) {
> + result = 1;
> goto out;
> + }
>
> if (verbose)
> printf("exporting %s:%s\n", exp->m_client->m_hostname,
> @@ -333,13 +379,15 @@ exportfs(char *arg, char *options, int verbose)
> exp->m_mayexport = 1;
> exp->m_changed = 1;
> exp->m_warned = 0;
> - validate_export(exp);
> + result = validate_export(exp);
>
> out:
> freeaddrinfo(ai);
> + return result;
> }
>
> -static void
> +/* return 0 on success */
> +static int
> unexportfs(char *arg, int verbose)
> {
> nfs_export *exp;
> @@ -347,13 +395,14 @@ unexportfs(char *arg, int verbose)
> char *path;
> char *hname = arg;
> int htype;
> + int result = 0;
>
> if ((path = strchr(arg, ':')) != NULL)
> *path++ = '\0';
>
> if (!path || *path != '/') {
> xlog(L_ERROR, "Invalid unexporting option: %s", arg);
> - return;
> + return 1;
> }
>
> if ((htype = client_gettype(hname)) == MCL_FQDN) {
> @@ -396,6 +445,7 @@ unexportfs(char *arg, int verbose)
> }
>
> freeaddrinfo(ai);
> + return result;
> }
>
> static int can_test(void)
> @@ -433,10 +483,11 @@ static int test_export(char *path, int with_fsid)
> return 1;
> }
>
> -static void
> +static int
> validate_export(nfs_export *exp)
> {
> - /* Check that the given export point is potentially exportable.
> + /* Check that the given export point is potentially exportable,
> + * returning 0 on success.
> * We just give warnings here, don't cause anything to fail.
> * If a path doesn't exist, or is not a dir or file, give an warning
> * otherwise trial-export to '-test-client-' and check for failure.
> @@ -448,15 +499,15 @@ validate_export(nfs_export *exp)
>
> if (stat(path, &stb) < 0) {
> xlog(L_ERROR, "Failed to stat %s: %m", path);
> - return;
> + return 1;
> }
> if (!S_ISDIR(stb.st_mode) && !S_ISREG(stb.st_mode)) {
> xlog(L_ERROR, "%s is neither a directory nor a file. "
> "Remote access will fail", path);
> - return;
> + return 1;
> }
> if (!can_test())
> - return;
> + return 1;
>
> if (!statfs64(path, &stf) &&
> (stf.f_fsid.__val[0] || stf.f_fsid.__val[1]))
> @@ -466,16 +517,16 @@ validate_export(nfs_export *exp)
> fs_has_fsid) {
> if ( !test_export(path, 1)) {
> xlog(L_ERROR, "%s does not support NFS export", path);
> - return;
> + return 1;
> }
> } else if ( ! test_export(path, 0)) {
> if (test_export(path, 1))
> xlog(L_ERROR, "%s requires fsid= for NFS export", path);
> else
> xlog(L_ERROR, "%s does not support NFS export", path);
> - return;
> -
> + return 1;
> }
> + return 0;
> }
>
> static _Bool
> --
> 1.7.12.4
>
_______________________________________________
Linux-HA mailing list
[email protected]
http://lists.linux-ha.org/mailman/listinfo/linux-ha
See also: http://linux-ha.org/ReportingProblems
