[PATCH 1/2] [REDIR] Move null redirect checks into caller
[REDIR] Move null redirect checks into caller
The null redirect checks were added as an optimisation to avoid
unnecessary memory allocations. However, we could avoid this
completely by simply making the caller avoid making a redirection
unless it is not null.
Signed-off-by: Herbert Xu herb...@gondor.apana.org.au
---
ChangeLog |1 +
src/eval.c |6 --
src/redir.c | 14 +-
3 files changed, 6 insertions(+), 15 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 4fc35a6..1fd184b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -2,6 +2,7 @@
* Fix poplocalvar on abnormal exit from function.
* Do not poplocalvars prematurely on regular utilities.
+ * Move null redirect checks into caller.
2010-05-26 Herbert Xu herb...@gondor.apana.org.au
diff --git a/src/eval.c b/src/eval.c
index 337667f..59bded9 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -224,7 +224,8 @@ evaltree(union node *n, int flags)
evaltree(n-nredir.n, flags EV_TESTED);
status = exitstatus;
}
- popredir(0);
+ if (n-nredir.redirect)
+ popredir(0);
goto setstatus;
case NCMD:
#ifdef notyet
@@ -879,7 +880,8 @@ raise:
}
out:
- popredir(execcmd);
+ if (cmd-ncmd.redirect)
+ popredir(execcmd);
unwindlocalvars(localvar_stop);
if (lastarg)
/* dsl: I think this is intended to be used to support
diff --git a/src/redir.c b/src/redir.c
index 54af96b..16decfc 100644
--- a/src/redir.c
+++ b/src/redir.c
@@ -72,12 +72,10 @@ MKINIT
struct redirtab {
struct redirtab *next;
int renamed[10];
- int nullredirs;
};
MKINIT struct redirtab *redirlist;
-MKINIT int nullredirs;
STATIC int openredirect(union node *);
#ifdef notyet
@@ -113,7 +111,6 @@ redirect(union node *redir, int flags)
memory[i] = 0;
memory[1] = flags REDIR_BACKQ;
#endif
- nullredirs++;
if (!redir) {
return;
}
@@ -124,10 +121,8 @@ redirect(union node *redir, int flags)
q = ckmalloc(sizeof (struct redirtab));
q-next = redirlist;
redirlist = q;
- q-nullredirs = nullredirs - 1;
for (i = 0 ; i 10 ; i++)
q-renamed[i] = EMPTY;
- nullredirs = 0;
sv = q;
}
n = redir;
@@ -343,8 +338,6 @@ popredir(int drop)
struct redirtab *rp;
int i;
- if (--nullredirs = 0)
- return;
INTOFF;
rp = redirlist;
for (i = 0 ; i 10 ; i++) {
@@ -364,7 +357,6 @@ popredir(int drop)
}
}
redirlist = rp-next;
- nullredirs = rp-nullredirs;
ckfree(rp);
INTON;
}
@@ -381,12 +373,8 @@ RESET {
/*
* Discard all saved file descriptors.
*/
- for (;;) {
- nullredirs = 0;
- if (!redirlist)
- break;
+ while (redirlist)
popredir(0);
- }
}
#endif
--
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: IFS regression in 55c46b7
On Thu, May 13, 2010 at 11:50:17PM +, Gerrit Pape wrote:
Hi, commit 55c46b7 introduces a regression in IFS handling, see
http://bugs.debian.org/581351
To reproduce, run
$ echo a:b |src/dash -c 'IFS=: read a b; echo $a $b'
with dash built from 55c46b7^:
$ echo a:b |src/dash -c 'IFS=: read a b; echo $a $b'
a b
with 55c46b7 and HEAD
$ echo a:b |src/dash -c 'IFS=: read a b; echo $a $b'
a:b
It appears to be fixed in the latest tree.
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} 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] [EVAL] Force fork if any trap is set, not just on EXIT.
Jilles Tjoelker jil...@stack.nl wrote:
In some cases the shell executes a subshell or an external command in
the current process. This is not done if a trap on EXIT has been set, so
that that trap can execute after the subshell or external command has
finished. Extend that check to all traps. (A trap is set if a
non-empty command string has been attached to it.)
Improve encapsulation by exporting an accessor function for this and
making the trap array static again.
This is much like FreeBSD SVN r194127, enhanced to apply to subshells
also (see FreeBSD SVN r194774).
Example:
dash -c '{ trap echo moo TERM; sleep 3; } sleep 1; kill $!;wait'
This should print moo after 3 seconds.
Example:
dash -c '{ trap echo moo TERM; (sleep 3) } sleep 1; kill $!;wait'
The same.
Example:
dash -c '{ trap echo moo TERM; sleep 3; :; } sleep 1; kill $!;wait'
This works correctly even without this patch.
Signed-off-by: Jilles Tjoelker jil...@stack.nl
I've applied it with some changes. In particuar, I've made the
have_traps code done using a count kept up-to-date by trapcmd
and clear_traps. Let me know if you see any problems with this.
diff --git a/ChangeLog b/ChangeLog
index 650899a..969d6be 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2010-05-27 Jilles Tjoelker jil...@stack.nl
+
+ * Force fork if any trap is set, not just on EXIT.
+
2010-05-27 Herbert Xu herb...@gondor.apana.org.au
* Fix poplocalvar on abnormal exit from function.
diff --git a/src/eval.c b/src/eval.c
index d5c1e6c..439f881 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -449,7 +449,7 @@ evalsubshell(union node *n, int flags)
int status;
expredir(n-nredir.redirect);
- if (!backgnd flags EV_EXIT !trap[0])
+ if (!backgnd flags EV_EXIT !have_traps())
goto nofork;
INTOFF;
jp = makejob(n, 1);
@@ -836,7 +836,7 @@ bail:
switch (cmdentry.cmdtype) {
default:
/* Fork off a child process if necessary. */
- if (!(flags EV_EXIT) || trap[0]) {
+ if (!(flags EV_EXIT) || have_traps()) {
INTOFF;
jp = makejob(cmd, 1);
if (forkshell(jp, cmd, FORK_FG) != 0) {
diff --git a/src/trap.c b/src/trap.c
index 3f93c46..ba32da6 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -69,7 +69,9 @@
/* trap handler commands */
-char *trap[NSIG];
+static char *trap[NSIG];
+/* number of non-null traps */
+int trapcnt;
/* current value of signal */
char sigmode[NSIG - 1];
/* indicates specified signal received */
@@ -125,11 +127,17 @@ trapcmd(int argc, char **argv)
if (action) {
if (action[0] == '-' action[1] == '\0')
action = NULL;
- else
+ else {
+ if (*action)
+ trapcnt++;
action = savestr(action);
+ }
}
- if (trap[signo])
+ if (trap[signo]) {
+ if (*trap[signo])
+ trapcnt--;
ckfree(trap[signo]);
+ }
trap[signo] = action;
if (signo != 0)
setsignal(signo);
@@ -150,16 +158,17 @@ clear_traps(void)
{
char **tp;
+ INTOFF;
for (tp = trap ; tp trap[NSIG] ; tp++) {
if (*tp **tp) { /* trap not NULL or SIG_IGN */
- INTOFF;
ckfree(*tp);
*tp = NULL;
if (tp != trap[0])
setsignal(tp - trap);
- INTON;
}
}
+ trapcnt = 0;
+ INTON;
}
diff --git a/src/trap.h b/src/trap.h
index f19a66b..50fc3ed 100644
--- a/src/trap.h
+++ b/src/trap.h
@@ -36,7 +36,7 @@
#include signal.h
-extern char *trap[];
+extern int trapcnt;
extern char sigmode[];
extern volatile sig_atomic_t pendingsigs;
@@ -49,3 +49,8 @@ int dotrap(void);
void setinteractive(int);
void exitshell(void) __attribute__((__noreturn__));
int decode_signal(const char *, int);
+
+static inline int have_traps(void)
+{
+ return trapcnt;
+}
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} 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
Job control bug in revision 3800d4934391b,
I'm not sure how to describe this bug, but it's affected one of my
scripts, and those of several of my users. Basically, we've had loops
dieing when backgrounded programs exit. This is the simplest test case
I can come up with:
#!/bin/dash
{
echo foo
sleep 1
echo foo
echo done/dev/tty
} | while read p; do
( echo good )
done
echo done
In versions prior to 3800d4934391b, the output would
good\ndone\ndone\ngood (or some permutation thereof depending on
system load), but from 3800d4934391b on, it's good\ndone.
The offending revision:
[JOBS] Fix dowait signal race
author Herbert Xu herb...@gondor.apana.org.au
Sun, 22 Feb 2009 10:10:01 + (18:10 +0800)
committer Herbert Xu herb...@gondor.apana.org.au
Sun, 22 Feb 2009 10:10:01 + (18:10 +0800)
commit 3800d4934391b144fd261a7957aea72ced7d47ea
tree40c003ab3063ceab7f3615a623a09d3c610332a0
parent 6045fe25078345074f027312d106d3fc19df56e5
[JOBS] Fix dowait signal race
This test program by Alexey Gladkov can cause dash to enter an
infinite loop in waitcmd.
#!/bin/dash
trap echo TRAP USR1
stub() {
echo STUB $1 2
sleep $1
echo STUB $1 2
kill -USR1 $$
}
stub 3
stub 2
until { echo ###; wait; } do
echo *** $?
done
The problem is that if we get a signal after the wait3 system
call has returned but before we get to INTON in dowait, then
we can jump back up to the top and lose the exit status. So
if we then wait for the job that has just exited, then it'll
stay there forever.
I made the original change that caused this bug to fix pretty
much the same bug but in the opposite direction. That is, if
we get a signal after we enter wait3 but before we hit the kernel
then it too can cause the wait to go on forever (assuming the
child doesn't exit).
In fact this is pretty much exactly the scenario that you'll
find in glibc's documentation on pause(). The solution is given
there too, in the form of sigsuspend, which is the only way to
do the check and wait atomically.
So this patch fixes Alexey's race without reintroducing the old
bug by converting the blocking wait3 to a sigsuspend.
In order to do this we need to set a signal handler for SIGCHLD,
so the code has been modified to always do that.
Signed-off-by: Herbert Xu herb...@gondor.apana.org.au
--
Kris Maglione
If you want to go somewhere, goto is the best way to get there.
--Ken Thompson
--
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
