Re: [PATCH 2/3] Revert Eliminated global exerrno.
On Wed, Oct 06, 2010 at 10:35:18PM -0500, Jonathan Nieder wrote: Maybe the following would make sense on top? -- 8 -- Subject: [EXCEPTIONS] Use EXEXIT in place of EXEXEC Patch applied. Thanks a lot! -- Email: Herbert Xu herb...@gondor.apana.org.au Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line unsubscribe dash in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] Revert Eliminated global exerrno.
Herbert Xu wrote: sh -c 'trap exec nosuchfile EXIT; exit 3'; echo $? This makes dash exit with 127 instead of 3 as it does now. AFAIK the wording of POSIX is such that we should return 3, however, it does seem that every other shell exits with 127. On 'exec': If exec is specified with command, it shall replace the shell with command without creating a new process. (1) If command is specified, exec shall not return to the shell; rather, the exit status of the process shall be the exit status of the program implementing command, which overlaid the shell. If command is not found, the exit status shall be 127. If command is found, but it is not an executable utility, the exit status shall be 126. If a redirection error occurs (see Consequences of Shell Errors ), the shell shall exit with a value in the range 1-125. Otherwise, exec shall return a zero exit status. (2) On 'exit': ... A trap on EXIT shall be executed before the shell terminates, except when the exit utility is invoked in that trap itself, in which case the shell shall exit immediately. (3) The exit status [of the exit utility -jrn] shall be n, if specified. Otherwise, the value shall be the exit value of the last command executed, or zero if no command was executed. When exit is executed in a trap action, the last command is considered to be the command that executed immediately preceding the trap action. (4) The passage (3) seems to mean that exit triggers an EXIT trap (except, to avoid having to deal with recursion, when the exit occurs within an EXIT trap). Then the exec builtin is invoked, resulting (according to (2)) in an exit(127) without returning to the shell. So I think the proposed behavior fits the spec. Regards, Jonathan -- To unsubscribe from this list: send the line unsubscribe dash in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] Revert Eliminated global exerrno.
This reverts commit c0e07c010e5abdea1a7d1357edb1d08adac529cb.
Yes, it is nicer to pass the exit status through the exitstatus
global, but look at the consequences:
$ sh -c 'exec nonexistent'; echo $?
exec: 1: nonexistent: not found
2
$ sh -c 'exec .'; echo $?
exec: 1: .: Permission denied
2
According to POSIX:
If command is not found, the exit status shall be 127. If command is
found, but it is not an executable utility, the exit status shall
be 126.
Make it so again.
Reported-by: Eric Blake ebl...@redhat.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
src/error.h |1 +
src/exec.c |3 +--
src/main.c |2 ++
3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/error.h b/src/error.h
index 15ccbe3..1bb6a82 100644
--- a/src/error.h
+++ b/src/error.h
@@ -62,6 +62,7 @@ struct jmploc {
extern struct jmploc *handler;
extern int exception;
+extern int exerrno;/* error for EXEXEC */
/* exceptions */
#define EXINT 0/* SIGINT received */
diff --git a/src/exec.c b/src/exec.c
index 42299ea..9525a16 100644
--- a/src/exec.c
+++ b/src/exec.c
@@ -86,6 +86,7 @@ struct tblentry {
STATIC struct tblentry *cmdtable[CMDTABLESIZE];
STATIC int builtinloc = -1;/* index in path of %builtin, or -1 */
+int exerrno; /* Last exec error */
STATIC void tryexec(char *, char **, char **);
@@ -108,7 +109,6 @@ shellexec(char **argv, const char *path, int idx)
char *cmdname;
int e;
char **envp;
- int exerrno;
envp = environment();
if (strchr(argv[0], '/') != NULL) {
@@ -138,7 +138,6 @@ shellexec(char **argv, const char *path, int idx)
exerrno = 2;
break;
}
- exitstatus = exerrno;
TRACE((shellexec failed for %s, errno %d, suppressint %d\n,
argv[0], e, suppressint ));
exerror(EXEXEC, %s: %s, argv[0], errmsg(e, E_EXEC));
diff --git a/src/main.c b/src/main.c
index 2bff956..5bc1a99 100644
--- a/src/main.c
+++ b/src/main.c
@@ -115,6 +115,8 @@ main(int argc, char **argv)
e = exception;
if (e == EXERROR)
exitstatus = 2;
+ else if (e == EXEXEC)
+ exitstatus = exerrno;
s = state;
if (e == EXEXIT || s == 0 || iflag == 0 || shlvl)
--
1.7.2.3
--
To unsubscribe from this list: send the line unsubscribe dash in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] Revert Eliminated global exerrno.
On Wed, Oct 06, 2010 at 08:04:58PM -0500, Jonathan Nieder wrote: This reverts commit c0e07c010e5abdea1a7d1357edb1d08adac529cb. Yes, it is nicer to pass the exit status through the exitstatus global, but look at the consequences: $ sh -c 'exec nonexistent'; echo $? exec: 1: nonexistent: not found 2 $ sh -c 'exec .'; echo $? exec: 1: .: Permission denied 2 According to POSIX: If command is not found, the exit status shall be 127. If command is found, but it is not an executable utility, the exit status shall be 126. Make it so again. Reported-by: Eric Blake ebl...@redhat.com Signed-off-by: Jonathan Nieder jrnie...@gmail.com Actually the bug is elsewhere. This patch works for me. commit 7f684260a2426ac61c06d2e4822429b00437ae24 Author: Herbert Xu herb...@gondor.apana.org.au Date: Thu Oct 7 10:55:15 2010 +0800 [BUILTIN] Fix EXEXEC status clobbering evalcommand always clobbers the exit status in case of an EXEXEC which means that exec always fails with exit status 2 regardless of what it actually returns. This patch adds the missing check for EXEXEC so that the correct exit status is preserved. Signed-off-by: Herbert Xu herb...@gondor.apana.org.au diff --git a/ChangeLog b/ChangeLog index 1dfe241..2faaedd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +2010-10-07 Herbert Xu herb...@gondor.apana.org.au + + * Fix EXEXEC status clobbering. + 2010-09-08 Herbert Xu herb...@gondor.apana.org.au * Fix ifsfirst/ifslastp leak. diff --git a/src/eval.c b/src/eval.c index 5b8d36b..b966749 100644 --- a/src/eval.c +++ b/src/eval.c @@ -854,7 +854,7 @@ bail: int i; i = exception; - if (i == EXEXIT) + if (i == EXEXIT || i == EXEXEC) goto raise; status = (i == EXINT) ? SIGINT + 128 : 2; Thanks, -- Email: Herbert Xu herb...@gondor.apana.org.au Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line unsubscribe dash in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] Revert Eliminated global exerrno.
Herbert Xu wrote: Actually the bug is elsewhere. It does bisect to there. :) But you're right, it would have been simpler to send one patch. --- a/src/eval.c +++ b/src/eval.c @@ -854,7 +854,7 @@ bail: int i; i = exception; - if (i == EXEXIT) + if (i == EXEXIT || i == EXEXEC) goto raise; Good call. This is better than my patch because it exits like it ought to for command exec nonexistent (as POSIX says: If command is specified, exec shall not return to the shell ). Maybe the following would make sense on top? -- 8 -- Subject: [EXCEPTIONS] Use EXEXIT in place of EXEXEC The intended semantics of EXEXEC are identical to EXEXIT, so simplify by using EXEXIT directly. Functional change: in edge cases (exec within a trap handler), this causes the exit status from exec not to be clobbered. For example, without this patch: $ sh -c 'trap exec nonexistent EXIT'; echo $? exec: 1: nonexistent: not found 0 And with it: $ sh -c 'trap exec nonexistent EXIT'; echo $? exec: 1: nonexistent: not found 127 Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- src/error.h |1 - src/eval.c |2 +- src/exec.c |2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/error.h b/src/error.h index be0eec9..f236d9f 100644 --- a/src/error.h +++ b/src/error.h @@ -66,7 +66,6 @@ extern int exception; /* exceptions */ #define EXINT 0/* SIGINT received */ #define EXERROR 1 /* a generic error */ -#define EXEXEC 3 /* command execution failed */ #define EXEXIT 4 /* exit the shell */ diff --git a/src/eval.c b/src/eval.c index b966749..5b8d36b 100644 --- a/src/eval.c +++ b/src/eval.c @@ -854,7 +854,7 @@ bail: int i; i = exception; - if (i == EXEXIT || i == EXEXEC) + if (i == EXEXIT) goto raise; status = (i == EXINT) ? SIGINT + 128 : 2; diff --git a/src/exec.c b/src/exec.c index 42299ea..b273420 100644 --- a/src/exec.c +++ b/src/exec.c @@ -141,7 +141,7 @@ shellexec(char **argv, const char *path, int idx) exitstatus = exerrno; TRACE((shellexec failed for %s, errno %d, suppressint %d\n, argv[0], e, suppressint )); - exerror(EXEXEC, %s: %s, argv[0], errmsg(e, E_EXEC)); + exerror(EXEXIT, %s: %s, argv[0], errmsg(e, E_EXEC)); /* NOTREACHED */ } -- 1.7.2.3 -- To unsubscribe from this list: send the line unsubscribe dash in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] Revert Eliminated global exerrno.
On Wed, Oct 06, 2010 at 10:35:18PM -0500, Jonathan Nieder wrote: Herbert Xu wrote: Actually the bug is elsewhere. It does bisect to there. :) But you're right, it would have been simpler to send one patch. --- a/src/eval.c +++ b/src/eval.c @@ -854,7 +854,7 @@ bail: int i; i = exception; - if (i == EXEXIT) + if (i == EXEXIT || i == EXEXEC) goto raise; Good call. This is better than my patch because it exits like it ought to for command exec nonexistent (as POSIX says: If command is specified, exec shall not return to the shell ). Maybe the following would make sense on top? Nice! I like where you are going with this :) Let me run it through the tests and apply it. Thanks, -- Email: Herbert Xu herb...@gondor.apana.org.au Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line unsubscribe dash in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] Revert Eliminated global exerrno.
On Thu, Oct 07, 2010 at 12:14:06PM +0800, Herbert Xu wrote: Let me run it through the tests and apply it. There is one problem with this and that is changing the exit status in a trap: sh -c 'trap exec nosuchfile EXIT; exit 3'; echo $? This makes dash exit with 127 instead of 3 as it does now. AFAIK the wording of POSIX is such that we should return 3, however, it does seem that every other shell exits with 127. So unless anyone objects, I will apply your patch and adopt this new behaviour. Thanks, -- Email: Herbert Xu herb...@gondor.apana.org.au Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line unsubscribe dash in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
