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 <[email protected]>
---
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
--
1.8.2.1
On 10/21/2013 05:25 PM, NeilBrown wrote:
> On Wed, 2 Oct 2013 18:29:44 -0500 Tony Asleson <[email protected]> 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 <[email protected]>
>
> 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?
The reason I chose to return values was to make sure requested operation
actually completed requested operation. Unexporting a non-existent
export is not considered an error and returns no indication you did
absolutely nothing.
When scripting exportfs from another program I wanted to know that the
operation I requested actually did what I asked so that I could catch
bad calls to it.
Regards,
Tony
On 23/10/13 19:31, Chuck Lever wrote:
>
> On Oct 23, 2013, at 6:18 PM, NeilBrown <[email protected]> wrote:
>
>> On Wed, 23 Oct 2013 12:36:23 -0500 Tony Asleson <[email protected]> wrote:
>>
>>> On 10/22/2013 08:44 PM, NeilBrown wrote:
>>>> On Tue, 22 Oct 2013 10:23:14 -0500 Tony Asleson <[email protected]> wrote:
>>>>> The reason I chose to return values was to make sure requested operation
>>>>> actually completed requested operation. Unexporting a non-existent
>>>>> export is not considered an error and returns no indication you did
>>>>> absolutely nothing.
>>>>
>>>> Hi,
>>>> thanks makes sense - I had missed that (even though you explained it in the
>>>> patch description :-( )
>>>>
>>>> With your patch, if asked to unexport something that wasn't exported it
>>>> would not report any error, but would exit with an error status. Is that
>>>> correct? I think I would rather have a message printed if there is an error.
>>>
>>> Correct, I only made changes for the exit status. I was trying to make
>>> changes that would be mostly invisible to end users. I have no concerns
>>> adding a printed error output too, but others may.
>>>
>>> Changing the behavior of any command line tool is potentially
>>> problematic when scripted.
>>>
>>>> So would something like this (on top of my patch) address you need, or was
>>>> there something else I missed?
>>>
>>> Yes, this should work for the unexport fs case.
>>>
>>> However, the reason my patch was a little more invasive was to ensure
>>> that both the export and unexport paths were covered.
>>>
>>> For example, if the strdup call fails in function client_init, we fail
>>> the operation and return exit value of 0. Unlikely, but just the first
>>> example I stumbled across.
>>
>> I think it is a lot closer to "impossible" than just "unlikely".
>> malloc doesn't fail in this sort of context, the OOM killer kills something
>> off instead.
>> My personal preference is to replace all malloc/calloc/strdup calls with
>> the xmalloc, xstrdup etc calls in support/nfs/xcommon.c.
>> If you are worried about malloc failing, I'd much prefer to see a patch which
>> changes nfs-utils to use those uniformly.
>>
>> There might be a question over the best behaviour for daemons like mountd
>> and gssd. However as we move towards having systemd manage those, they will
>> be restarted if they ever exit, and they are mostly stateless so that is
>> quite safe.
>>
>> Does anyone else have thoughts on this?
>
> Yes. My thought is "xmalloc is an abomination." :-)
>
> We really do not want any of these tools exiting left if there's a memory allocation failure.
> For a user, that's no better than a segfault.
I the past I have agreed with this... But as Neil points out, we now live in
a systemd world were daemons are restarted, so maybe it does make sense to
exit on these types of failures. With daemons like mountd there is
really no state that would be lost....
steved.
>
> What's more, if a utility like exportfs isn't very carefully coded, a sideways exit
> can leave on-disk files in an inconsistent state.
>
> A rule of thumb is never hide control flow (like exiting) inside macros or libraries.
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
>
>
>
On 02/10/13 19:29, 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 <[email protected]>
Committed!
steved.
> ---
> 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
>
On Wed, 23 Oct 2013 12:36:23 -0500 Tony Asleson <[email protected]> wrote:
> On 10/22/2013 08:44 PM, NeilBrown wrote:
> > On Tue, 22 Oct 2013 10:23:14 -0500 Tony Asleson <[email protected]> wrote:
> >> The reason I chose to return values was to make sure requested operation
> >> actually completed requested operation. Unexporting a non-existent
> >> export is not considered an error and returns no indication you did
> >> absolutely nothing.
> >
> > Hi,
> > thanks makes sense - I had missed that (even though you explained it in the
> > patch description :-( )
> >
> > With your patch, if asked to unexport something that wasn't exported it
> > would not report any error, but would exit with an error status. Is that
> > correct? I think I would rather have a message printed if there is an error.
>
> Correct, I only made changes for the exit status. I was trying to make
> changes that would be mostly invisible to end users. I have no concerns
> adding a printed error output too, but others may.
>
> Changing the behavior of any command line tool is potentially
> problematic when scripted.
>
> > So would something like this (on top of my patch) address you need, or was
> > there something else I missed?
>
> Yes, this should work for the unexport fs case.
>
> However, the reason my patch was a little more invasive was to ensure
> that both the export and unexport paths were covered.
>
> For example, if the strdup call fails in function client_init, we fail
> the operation and return exit value of 0. Unlikely, but just the first
> example I stumbled across.
I think it is a lot closer to "impossible" than just "unlikely".
malloc doesn't fail in this sort of context, the OOM killer kills something
off instead.
My personal preference is to replace all malloc/calloc/strdup calls with
the xmalloc, xstrdup etc calls in support/nfs/xcommon.c.
If you are worried about malloc failing, I'd much prefer to see a patch which
changes nfs-utils to use those uniformly.
There might be a question over the best behaviour for daemons like mountd
and gssd. However as we move towards having systemd manage those, they will
be restarted if they ever exit, and they are mostly stateless so that is
quite safe.
Does anyone else have thoughts on this?
NeilBrown
On Tue, 22 Oct 2013 04:36:24 -0400 Steve Dickson <[email protected]> wrote:
>
>
> On 22/10/13 04:30, Steve Dickson wrote:
> >
> >
> > On 02/10/13 19:29, 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 <[email protected]>
> > Committed!
> Unfortunately I did not see Neil's patch before I committed this patch....
> So I'm going to revert this patch in favor of Neil's...
>
> steved.
Hi Steve,
it doesn't look like you achieved the result you were after.
commit 956aeff2e24304e938846f81f4b9db34cbf18a32
is Tony's original patch.
commit efe3c8d6cb4fc35909a64c0535087676a189fa5f
reverts it.
commit 232eb7ad09f9fd2ae4918699f850e4f8cadc2632
apples Tony's original patch again, but is credited
to me with my patch description.
So something is a bit messed up.
That is why my subsequent
exportfs: report failure if asked to unexport something not exported.
didn't apply.
NeilBrown
On Oct 24, 2013, at 11:56 AM, Steve Dickson <[email protected]> wrote:
>
>
> On 23/10/13 19:31, Chuck Lever wrote:
>>
>> On Oct 23, 2013, at 6:18 PM, NeilBrown <[email protected]> wrote:
>>
>>> On Wed, 23 Oct 2013 12:36:23 -0500 Tony Asleson <[email protected]> wrote:
>>>
>>>> On 10/22/2013 08:44 PM, NeilBrown wrote:
>>>>> On Tue, 22 Oct 2013 10:23:14 -0500 Tony Asleson <[email protected]> wrote:
>>>>>> The reason I chose to return values was to make sure requested operation
>>>>>> actually completed requested operation. Unexporting a non-existent
>>>>>> export is not considered an error and returns no indication you did
>>>>>> absolutely nothing.
>>>>>
>>>>> Hi,
>>>>> thanks makes sense - I had missed that (even though you explained it in the
>>>>> patch description :-( )
>>>>>
>>>>> With your patch, if asked to unexport something that wasn't exported it
>>>>> would not report any error, but would exit with an error status. Is that
>>>>> correct? I think I would rather have a message printed if there is an error.
>>>>
>>>> Correct, I only made changes for the exit status. I was trying to make
>>>> changes that would be mostly invisible to end users. I have no concerns
>>>> adding a printed error output too, but others may.
>>>>
>>>> Changing the behavior of any command line tool is potentially
>>>> problematic when scripted.
>>>>
>>>>> So would something like this (on top of my patch) address you need, or was
>>>>> there something else I missed?
>>>>
>>>> Yes, this should work for the unexport fs case.
>>>>
>>>> However, the reason my patch was a little more invasive was to ensure
>>>> that both the export and unexport paths were covered.
>>>>
>>>> For example, if the strdup call fails in function client_init, we fail
>>>> the operation and return exit value of 0. Unlikely, but just the first
>>>> example I stumbled across.
>>>
>>> I think it is a lot closer to "impossible" than just "unlikely".
>>> malloc doesn't fail in this sort of context, the OOM killer kills something
>>> off instead.
>>> My personal preference is to replace all malloc/calloc/strdup calls with
>>> the xmalloc, xstrdup etc calls in support/nfs/xcommon.c.
>>> If you are worried about malloc failing, I'd much prefer to see a patch which
>>> changes nfs-utils to use those uniformly.
>>>
>>> There might be a question over the best behaviour for daemons like mountd
>>> and gssd. However as we move towards having systemd manage those, they will
>>> be restarted if they ever exit, and they are mostly stateless so that is
>>> quite safe.
>>>
>>> Does anyone else have thoughts on this?
>>
>> Yes. My thought is "xmalloc is an abomination." :-)
>>
>> We really do not want any of these tools exiting left if there's a memory allocation failure.
>> For a user, that's no better than a segfault.
> I the past I have agreed with this... But as Neil points out, we now live in
> a systemd world were daemons are restarted, so maybe it does make sense to
> exit on these types of failures. With daemons like mountd there is
> really no state that would be lost....
Neil's arguments are very practical, but ...
There are other reasons that malloc() can fail. Software bugs are high on that list. It can also fail if user input (or network input) is used to determine the requested allocation size.
In addition, rpmlint/fedpkg-lint complain if there's an exit(2) call in your linked libraries. They would frown on xmalloc() invoking exit (they also aren't happy with xlog).
Whether or not it's OK for daemons, I still maintain that for administrative tools run directly by users like exportfs, we want to be more careful. Since the daemons share the same libraries as the user tools, that means xmalloc and friends should be avoided everywhere, IMO.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Oct 27, 2013, at 11:39 PM, NeilBrown <[email protected]> wrote:
> On Thu, 24 Oct 2013 12:05:35 -0400 Chuck Lever <[email protected]> wrote:
>
>>
>> On Oct 24, 2013, at 11:56 AM, Steve Dickson <[email protected]> wrote:
>>
>>>
>>>
>>> On 23/10/13 19:31, Chuck Lever wrote:
>>>>
>>>> On Oct 23, 2013, at 6:18 PM, NeilBrown <[email protected]> wrote:
>>>>
>>>>> On Wed, 23 Oct 2013 12:36:23 -0500 Tony Asleson <[email protected]> wrote:
>>>>>
>>>>>> On 10/22/2013 08:44 PM, NeilBrown wrote:
>>>>>>> On Tue, 22 Oct 2013 10:23:14 -0500 Tony Asleson <[email protected]> wrote:
>>>>>>>> The reason I chose to return values was to make sure requested operation
>>>>>>>> actually completed requested operation. Unexporting a non-existent
>>>>>>>> export is not considered an error and returns no indication you did
>>>>>>>> absolutely nothing.
>>>>>>>
>>>>>>> Hi,
>>>>>>> thanks makes sense - I had missed that (even though you explained it in the
>>>>>>> patch description :-( )
>>>>>>>
>>>>>>> With your patch, if asked to unexport something that wasn't exported it
>>>>>>> would not report any error, but would exit with an error status. Is that
>>>>>>> correct? I think I would rather have a message printed if there is an error.
>>>>>>
>>>>>> Correct, I only made changes for the exit status. I was trying to make
>>>>>> changes that would be mostly invisible to end users. I have no concerns
>>>>>> adding a printed error output too, but others may.
>>>>>>
>>>>>> Changing the behavior of any command line tool is potentially
>>>>>> problematic when scripted.
>>>>>>
>>>>>>> So would something like this (on top of my patch) address you need, or was
>>>>>>> there something else I missed?
>>>>>>
>>>>>> Yes, this should work for the unexport fs case.
>>>>>>
>>>>>> However, the reason my patch was a little more invasive was to ensure
>>>>>> that both the export and unexport paths were covered.
>>>>>>
>>>>>> For example, if the strdup call fails in function client_init, we fail
>>>>>> the operation and return exit value of 0. Unlikely, but just the first
>>>>>> example I stumbled across.
>>>>>
>>>>> I think it is a lot closer to "impossible" than just "unlikely".
>>>>> malloc doesn't fail in this sort of context, the OOM killer kills something
>>>>> off instead.
>>>>> My personal preference is to replace all malloc/calloc/strdup calls with
>>>>> the xmalloc, xstrdup etc calls in support/nfs/xcommon.c.
>>>>> If you are worried about malloc failing, I'd much prefer to see a patch which
>>>>> changes nfs-utils to use those uniformly.
>>>>>
>>>>> There might be a question over the best behaviour for daemons like mountd
>>>>> and gssd. However as we move towards having systemd manage those, they will
>>>>> be restarted if they ever exit, and they are mostly stateless so that is
>>>>> quite safe.
>>>>>
>>>>> Does anyone else have thoughts on this?
>>>>
>>>> Yes. My thought is "xmalloc is an abomination." :-)
>>>>
>>>> We really do not want any of these tools exiting left if there's a memory allocation failure.
>>>> For a user, that's no better than a segfault.
>>> I the past I have agreed with this... But as Neil points out, we now live in
>>> a systemd world were daemons are restarted, so maybe it does make sense to
>>> exit on these types of failures. With daemons like mountd there is
>>> really no state that would be lost....
>>
>> Neil's arguments are very practical, but ...
>>
>> There are other reasons that malloc() can fail. Software bugs are high on that list. It can also fail if user input (or network input) is used to determine the requested allocation size.
>>
>> In addition, rpmlint/fedpkg-lint complain if there's an exit(2) call in your linked libraries. They would frown on xmalloc() invoking exit (they also aren't happy with xlog).
>>
>> Whether or not it's OK for daemons, I still maintain that for administrative tools run directly by users like exportfs, we want to be more careful. Since the daemons share the same libraries as the user tools, that means xmalloc and friends should be avoided everywhere, IMO.
>
> I don't follow this argument. Why do we need to be more careful for
> administrative tools?
>
> Tools should always be written to be crash-proof, and I believe exportfs is.
> It writes to a temp file and then performs an atomic rename when the new file
> is ready. If anything goes wrong it is perfectly safe to simply exit, and
> the important files will be unchanged.
Data corruption is only half the argument. The other half is how this looks to users when it happens. The tool just stops working with a cryptic error message. IMO it looks no better than a segfault.
But you may be right; there might be no good alternatives, and the possibility of failure is rare in normal cases.
> The memory allocation failures that we are talking about here are for a dozen
> bytes or so and are extremely are. I would be a lot more confident in 'exit'
> doing the right thing, than in multiple untested error paths carrying the
> error up and making sure not to write out the file if the malloc error might
> result in it having the wrong value.
As you say, this isn't an issue if the tool is writing to a temp file, then renaming as the last step.
> (On the question of 'exit' in libraries, I'm ambivalent).
While this doesn't apply to exportfs, of course, these days one may also have to think about GUIs, binding to other language environments, or scripts run in "behind the scenes" environments where there is no possibility of reporting an error. Sideways library exits are quite unhelpful in those cases. They might make prototyping a little easier, but I can't think of a good user-centric reason to use them.
But, I'm not objecting, just whinging a bit. If you want to take this course, then go ahead. It's really an argument about coding style rather than something important ;-)
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On 10/23/2013 05:18 PM, NeilBrown wrote:
> On Wed, 23 Oct 2013 12:36:23 -0500 Tony Asleson <[email protected]> wrote:
>> For example, if the strdup call fails in function client_init, we fail
>> the operation and return exit value of 0. Unlikely, but just the first
>> example I stumbled across.
>
> I think it is a lot closer to "impossible" than just "unlikely".
> malloc doesn't fail in this sort of context, the OOM killer kills something
> off instead.
> My personal preference is to replace all malloc/calloc/strdup calls with
> the xmalloc, xstrdup etc calls in support/nfs/xcommon.c.
> If you are worried about malloc failing, I'd much prefer to see a patch which
> changes nfs-utils to use those uniformly.
Sorry, my real point was that there are other ways for exportfs to fail
to do the operation and fail to report it, thus the reason my patch was
more invasive. I will try to find another more likely example that
illustrates this.
Regards,
Tony
On Oct 23, 2013, at 6:18 PM, NeilBrown <[email protected]> wrote:
> On Wed, 23 Oct 2013 12:36:23 -0500 Tony Asleson <[email protected]> wrote:
>
>> On 10/22/2013 08:44 PM, NeilBrown wrote:
>>> On Tue, 22 Oct 2013 10:23:14 -0500 Tony Asleson <[email protected]> wrote:
>>>> The reason I chose to return values was to make sure requested operation
>>>> actually completed requested operation. Unexporting a non-existent
>>>> export is not considered an error and returns no indication you did
>>>> absolutely nothing.
>>>
>>> Hi,
>>> thanks makes sense - I had missed that (even though you explained it in the
>>> patch description :-( )
>>>
>>> With your patch, if asked to unexport something that wasn't exported it
>>> would not report any error, but would exit with an error status. Is that
>>> correct? I think I would rather have a message printed if there is an error.
>>
>> Correct, I only made changes for the exit status. I was trying to make
>> changes that would be mostly invisible to end users. I have no concerns
>> adding a printed error output too, but others may.
>>
>> Changing the behavior of any command line tool is potentially
>> problematic when scripted.
>>
>>> So would something like this (on top of my patch) address you need, or was
>>> there something else I missed?
>>
>> Yes, this should work for the unexport fs case.
>>
>> However, the reason my patch was a little more invasive was to ensure
>> that both the export and unexport paths were covered.
>>
>> For example, if the strdup call fails in function client_init, we fail
>> the operation and return exit value of 0. Unlikely, but just the first
>> example I stumbled across.
>
> I think it is a lot closer to "impossible" than just "unlikely".
> malloc doesn't fail in this sort of context, the OOM killer kills something
> off instead.
> My personal preference is to replace all malloc/calloc/strdup calls with
> the xmalloc, xstrdup etc calls in support/nfs/xcommon.c.
> If you are worried about malloc failing, I'd much prefer to see a patch which
> changes nfs-utils to use those uniformly.
>
> There might be a question over the best behaviour for daemons like mountd
> and gssd. However as we move towards having systemd manage those, they will
> be restarted if they ever exit, and they are mostly stateless so that is
> quite safe.
>
> Does anyone else have thoughts on this?
Yes. My thought is "xmalloc is an abomination." :-)
We really do not want any of these tools exiting left if there's a memory allocation failure. For a user, that's no better than a segfault.
What's more, if a utility like exportfs isn't very carefully coded, a sideways exit can leave on-disk files in an inconsistent state.
A rule of thumb is never hide control flow (like exiting) inside macros or libraries.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On 21/10/13 18:25, NeilBrown wrote:
> From 2f63d6c3c38e673216a3351aff47d32059056638 Mon Sep 17 00:00:00 2001
> From: Neil Brown <[email protected]>
> 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 <[email protected]>
Committed...
steved.
>
> 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':
On Wed, 2 Oct 2013 18:29:44 -0500 Tony Asleson <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
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':
On Thu, 24 Oct 2013 12:05:35 -0400 Chuck Lever <[email protected]> wrote:
>
> On Oct 24, 2013, at 11:56 AM, Steve Dickson <[email protected]> wrote:
>
> >
> >
> > On 23/10/13 19:31, Chuck Lever wrote:
> >>
> >> On Oct 23, 2013, at 6:18 PM, NeilBrown <[email protected]> wrote:
> >>
> >>> On Wed, 23 Oct 2013 12:36:23 -0500 Tony Asleson <[email protected]> wrote:
> >>>
> >>>> On 10/22/2013 08:44 PM, NeilBrown wrote:
> >>>>> On Tue, 22 Oct 2013 10:23:14 -0500 Tony Asleson <[email protected]> wrote:
> >>>>>> The reason I chose to return values was to make sure requested operation
> >>>>>> actually completed requested operation. Unexporting a non-existent
> >>>>>> export is not considered an error and returns no indication you did
> >>>>>> absolutely nothing.
> >>>>>
> >>>>> Hi,
> >>>>> thanks makes sense - I had missed that (even though you explained it in the
> >>>>> patch description :-( )
> >>>>>
> >>>>> With your patch, if asked to unexport something that wasn't exported it
> >>>>> would not report any error, but would exit with an error status. Is that
> >>>>> correct? I think I would rather have a message printed if there is an error.
> >>>>
> >>>> Correct, I only made changes for the exit status. I was trying to make
> >>>> changes that would be mostly invisible to end users. I have no concerns
> >>>> adding a printed error output too, but others may.
> >>>>
> >>>> Changing the behavior of any command line tool is potentially
> >>>> problematic when scripted.
> >>>>
> >>>>> So would something like this (on top of my patch) address you need, or was
> >>>>> there something else I missed?
> >>>>
> >>>> Yes, this should work for the unexport fs case.
> >>>>
> >>>> However, the reason my patch was a little more invasive was to ensure
> >>>> that both the export and unexport paths were covered.
> >>>>
> >>>> For example, if the strdup call fails in function client_init, we fail
> >>>> the operation and return exit value of 0. Unlikely, but just the first
> >>>> example I stumbled across.
> >>>
> >>> I think it is a lot closer to "impossible" than just "unlikely".
> >>> malloc doesn't fail in this sort of context, the OOM killer kills something
> >>> off instead.
> >>> My personal preference is to replace all malloc/calloc/strdup calls with
> >>> the xmalloc, xstrdup etc calls in support/nfs/xcommon.c.
> >>> If you are worried about malloc failing, I'd much prefer to see a patch which
> >>> changes nfs-utils to use those uniformly.
> >>>
> >>> There might be a question over the best behaviour for daemons like mountd
> >>> and gssd. However as we move towards having systemd manage those, they will
> >>> be restarted if they ever exit, and they are mostly stateless so that is
> >>> quite safe.
> >>>
> >>> Does anyone else have thoughts on this?
> >>
> >> Yes. My thought is "xmalloc is an abomination." :-)
> >>
> >> We really do not want any of these tools exiting left if there's a memory allocation failure.
> >> For a user, that's no better than a segfault.
> > I the past I have agreed with this... But as Neil points out, we now live in
> > a systemd world were daemons are restarted, so maybe it does make sense to
> > exit on these types of failures. With daemons like mountd there is
> > really no state that would be lost....
>
> Neil's arguments are very practical, but ...
>
> There are other reasons that malloc() can fail. Software bugs are high on that list. It can also fail if user input (or network input) is used to determine the requested allocation size.
>
> In addition, rpmlint/fedpkg-lint complain if there's an exit(2) call in your linked libraries. They would frown on xmalloc() invoking exit (they also aren't happy with xlog).
>
> Whether or not it's OK for daemons, I still maintain that for administrative tools run directly by users like exportfs, we want to be more careful. Since the daemons share the same libraries as the user tools, that means xmalloc and friends should be avoided everywhere, IMO.
I don't follow this argument. Why do we need to be more careful for
administrative tools?
Tools should always be written to be crash-proof, and I believe exportfs is.
It writes to a temp file and then performs an atomic rename when the new file
is ready. If anything goes wrong it is perfectly safe to simply exit, and
the important files will be unchanged.
The memory allocation failures that we are talking about here are for a dozen
bytes or so and are extremely are. I would be a lot more confident in 'exit'
doing the right thing, than in multiple untested error paths carrying the
error up and making sure not to write out the file if the malloc error might
result in it having the wrong value.
(On the question of 'exit' in libraries, I'm ambivalent).
NeilBrown
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
>
>
On 10/22/2013 08:44 PM, NeilBrown wrote:
> On Tue, 22 Oct 2013 10:23:14 -0500 Tony Asleson <[email protected]> wrote:
>> The reason I chose to return values was to make sure requested operation
>> actually completed requested operation. Unexporting a non-existent
>> export is not considered an error and returns no indication you did
>> absolutely nothing.
>
> Hi,
> thanks makes sense - I had missed that (even though you explained it in the
> patch description :-( )
>
> With your patch, if asked to unexport something that wasn't exported it
> would not report any error, but would exit with an error status. Is that
> correct? I think I would rather have a message printed if there is an error.
Correct, I only made changes for the exit status. I was trying to make
changes that would be mostly invisible to end users. I have no concerns
adding a printed error output too, but others may.
Changing the behavior of any command line tool is potentially
problematic when scripted.
> So would something like this (on top of my patch) address you need, or was
> there something else I missed?
Yes, this should work for the unexport fs case.
However, the reason my patch was a little more invasive was to ensure
that both the export and unexport paths were covered.
For example, if the strdup call fails in function client_init, we fail
the operation and return exit value of 0. Unlikely, but just the first
example I stumbled across.
Thanks,
Tony
On Tue, 22 Oct 2013 10:23:14 -0500 Tony Asleson <[email protected]> wrote:
> On 10/21/2013 05:25 PM, NeilBrown wrote:
> > On Wed, 2 Oct 2013 18:29:44 -0500 Tony Asleson <[email protected]> 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 <[email protected]>
> >
> > 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?
>
> The reason I chose to return values was to make sure requested operation
> actually completed requested operation. Unexporting a non-existent
> export is not considered an error and returns no indication you did
> absolutely nothing.
Hi,
thanks makes sense - I had missed that (even though you explained it in the
patch description :-( )
With your patch, if asked to unexport something that wasn't exported it
would not report any error, but would exit with an error status. Is that
correct? I think I would rather have a message printed if there is an error.
So would something like this (on top of my patch) address you need, or was
there something else I missed?
Thanks,
NeilBrown
diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
index 52fc03d..c9e12db 100644
--- a/utils/exportfs/exportfs.c
+++ b/utils/exportfs/exportfs.c
@@ -351,6 +351,7 @@ unexportfs(char *arg, int verbose)
char *path;
char *hname = arg;
int htype;
+ int success = 0;
if ((path = strchr(arg, ':')) != NULL)
*path++ = '\0';
@@ -397,7 +398,10 @@ unexportfs(char *arg, int verbose)
#endif
exp->m_xtabent = 0;
exp->m_mayexport = 0;
+ success = 1;
}
+ if (!success)
+ xlog(L_ERROR, "Could not find %s to unexport.\n", arg);
freeaddrinfo(ai);
}
On 22/10/13 04:30, Steve Dickson wrote:
>
>
> On 02/10/13 19:29, 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 <[email protected]>
> Committed!
Unfortunately I did not see Neil's patch before I committed this patch....
So I'm going to revert this patch in favor of Neil's...
steved.
>
> steved.
>
>> ---
>> 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
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 28/10/13 18:35, NeilBrown wrote:
> On Tue, 22 Oct 2013 04:36:24 -0400 Steve Dickson <[email protected]> wrote:
>
>>
>>
>> On 22/10/13 04:30, Steve Dickson wrote:
>>>
>>>
>>> On 02/10/13 19:29, 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 <[email protected]>
>>> Committed!
>> Unfortunately I did not see Neil's patch before I committed this patch....
>> So I'm going to revert this patch in favor of Neil's...
>>
>> steved.
>
> Hi Steve,
>
> it doesn't look like you achieved the result you were after.
>
> commit 956aeff2e24304e938846f81f4b9db34cbf18a32
> is Tony's original patch.
> commit efe3c8d6cb4fc35909a64c0535087676a189fa5f
> reverts it.
>
> commit 232eb7ad09f9fd2ae4918699f850e4f8cadc2632
> apples Tony's original patch again, but is credited
> to me with my patch description.
>
> So something is a bit messed up.
> That is why my subsequent
> exportfs: report failure if asked to unexport something not exported.
>
> didn't apply.
Yea... I had a tough week last week... Let me sort this out and do the right
thing...
steved.
>
> NeilBrown
>