Zsh Mailing List Archive
Messages sorted by:
Reverse Date,
Date,
Thread,
Author
Re: Strange parameter visibility
On Fri, 30 Sep 2016 10:36:25 +0100
Peter Stephenson <p.stephenson@xxxxxxxxxxx> wrote:
> A more robust fix is a complete refactoring of execcmd(), which is far
> too big anyway, with the appropriate parts being called form above,
> deciding on asignments and analysing the commands early, possibly
> allowing there to be a single fork in one place. However, that's a big
> job --- and without extra code to delay prefork() I still don't think
> that fixes the case ": ${x:=2} | echo $x", and I suspect the case
> "cmd=:; $cmd ${x:=2} | echo $x" is pretty much unfixable without
> delaying the assignment from the ${x:=2} until a later part of
> processing, which might have side effects.
A start at refactoring, which I think is generally beneficial, already
fixes the original problem robustly with a small tweak: we can now
check within execpline2() that there are no arguments to the command,
so therefore it will be handled entirely within the shell.
This probably needs more exercising, but tests already passed (including
the new one which is restored).
If you thought "but I really liked the huge, monolithic execcmd()" please
keep your thoughts to yourself...
pws
diff --git a/Src/exec.c b/Src/exec.c
index 2714edb..349414c 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1808,6 +1808,7 @@ execpline2(Estate state, wordcode pcode,
{
pid_t pid;
int pipes[2];
+ struct execcmd_params eparams;
if (breaks || retflag)
return;
@@ -1825,17 +1826,16 @@ execpline2(Estate state, wordcode pcode,
else
list_pipe_text[0] = '\0';
}
- if (WC_PIPE_TYPE(pcode) == WC_PIPE_END)
- execcmd(state, input, output, how, last1 ? 1 : 2);
- else {
+ if (WC_PIPE_TYPE(pcode) == WC_PIPE_END) {
+ execcmd_analyse(state, &eparams);
+ execcmd_exec(state, &eparams, input, output, how, last1 ? 1 : 2);
+ } else {
int old_list_pipe = list_pipe;
int subsh_close = -1;
- Wordcode next = state->pc + (*state->pc), pc;
- wordcode code;
+ Wordcode next = state->pc + (*state->pc), start_pc;
- state->pc++;
- for (pc = state->pc; wc_code(code = *pc) == WC_REDIR;
- pc += WC_REDIR_WORDS(code));
+ start_pc = ++state->pc;
+ execcmd_analyse(state, &eparams);
if (mpipe(pipes) < 0) {
/* FIXME */
@@ -1844,7 +1844,7 @@ execpline2(Estate state, wordcode pcode,
/* if we are doing "foo | bar" where foo is a current *
* shell command, do foo in a subshell and do the *
* rest of the pipeline in the current shell. */
- if ((wc_code(code) >= WC_CURSH)
+ if ((eparams.type >= WC_CURSH || !eparams.args)
&& (how & Z_SYNC)) {
int synch[2];
struct timeval bgtime;
@@ -1863,7 +1863,7 @@ execpline2(Estate state, wordcode pcode,
} else if (pid) {
char dummy, *text;
- text = getjobtext(state->prog, state->pc);
+ text = getjobtext(state->prog, start_pc);
addproc(pid, text, 0, &bgtime);
close(synch[1]);
read_loop(synch[0], &dummy, 1);
@@ -1874,14 +1874,14 @@ execpline2(Estate state, wordcode pcode,
entersubsh(((how & Z_ASYNC) ? ESUB_ASYNC : 0)
| ESUB_PGRP | ESUB_KEEPTRAP);
close(synch[1]);
- execcmd(state, input, pipes[1], how, 1);
+ execcmd_exec(state, &eparams, input, pipes[1], how, 1);
_exit(lastval);
}
} else {
/* otherwise just do the pipeline normally. */
addfilelist(NULL, pipes[0]);
subsh_close = pipes[0];
- execcmd(state, input, pipes[1], how, 0);
+ execcmd_exec(state, &eparams, input, pipes[1], how, 0);
}
zclose(pipes[1]);
state->pc = next;
@@ -2537,55 +2537,51 @@ resolvebuiltin(const char *cmdarg, HashNode hn)
return hn;
}
+/*
+ * We are about to execute a command at the lowest level of the
+ * hierarchy. Analyse the parameters from the wordcode.
+ */
+
/**/
static void
-execcmd(Estate state, int input, int output, int how, int last1)
+execcmd_analyse(Estate state, Execcmd_params eparams)
{
- HashNode hn = NULL;
- LinkList args, filelist = NULL;
- LinkNode node;
- Redir fn;
- struct multio *mfds[10];
- char *text;
- int save[10];
- int fil, dfil, is_cursh, type, do_exec = 0, redir_err = 0, i, htok = 0;
- int nullexec = 0, assign = 0, forked = 0, postassigns = 0;
- int is_shfunc = 0, is_builtin = 0, is_exec = 0, use_defpath = 0;
- /* Various flags to the command. */
- int cflags = 0, orig_cflags = 0, checked = 0, oautocont = -1;
- LinkList redir;
wordcode code;
- Wordcode beg = state->pc, varspc, assignspc = (Wordcode)0;
- FILE *oxtrerr = xtrerr, *newxtrerr = NULL;
+ int i;
- doneps4 = 0;
- redir = (wc_code(*state->pc) == WC_REDIR ? ecgetredirs(state) : NULL);
+ eparams->beg = state->pc;
+ eparams->redir =
+ (wc_code(*state->pc) == WC_REDIR ? ecgetredirs(state) : NULL);
if (wc_code(*state->pc) == WC_ASSIGN) {
cmdoutval = 0;
- varspc = state->pc;
+ eparams->varspc = state->pc;
while (wc_code((code = *state->pc)) == WC_ASSIGN)
state->pc += (WC_ASSIGN_TYPE(code) == WC_ASSIGN_SCALAR ?
3 : WC_ASSIGN_NUM(code) + 2);
} else
- varspc = NULL;
+ eparams->varspc = NULL;
code = *state->pc++;
- type = wc_code(code);
+ eparams->type = wc_code(code);
+ eparams->postassigns = 0;
/* It would be nice if we could use EC_DUPTOK instead of EC_DUP here.
* But for that we would need to check/change all builtins so that
* they don't modify their argument strings. */
- switch (type) {
+ switch (eparams->type) {
case WC_SIMPLE:
- args = ecgetlist(state, WC_SIMPLE_ARGC(code), EC_DUP, &htok);
+ eparams->args = ecgetlist(state, WC_SIMPLE_ARGC(code), EC_DUP,
+ &eparams->htok);
+ eparams->assignspc = NULL;
break;
case WC_TYPESET:
- args = ecgetlist(state, WC_TYPESET_ARGC(code), EC_DUP, &htok);
- postassigns = *state->pc++;
- assignspc = state->pc;
- for (i = 0; i < postassigns; i++) {
+ eparams->args = ecgetlist(state, WC_TYPESET_ARGC(code), EC_DUP,
+ &eparams->htok);
+ eparams->postassigns = *state->pc++;
+ eparams->assignspc = state->pc;
+ for (i = 0; i < eparams->postassigns; i++) {
code = *state->pc;
DPUTS(wc_code(code) != WC_ASSIGN,
"BUG: miscounted typeset assignments");
@@ -2595,8 +2591,45 @@ execcmd(Estate state, int input, int output, int how, int last1)
break;
default:
- args = NULL;
+ eparams->args = NULL;
+ eparams->assignspc = NULL;
+ eparams->htok = 0;
+ break;
}
+}
+
+/*
+ * Execute a command at the lowest level of the hierarchy.
+ */
+
+/**/
+static void
+execcmd_exec(Estate state, Execcmd_params eparams,
+ int input, int output, int how, int last1)
+{
+ HashNode hn = NULL;
+ LinkList filelist = NULL;
+ LinkNode node;
+ Redir fn;
+ struct multio *mfds[10];
+ char *text;
+ int save[10];
+ int fil, dfil, is_cursh, do_exec = 0, redir_err = 0, i;
+ int nullexec = 0, assign = 0, forked = 0;
+ int is_shfunc = 0, is_builtin = 0, is_exec = 0, use_defpath = 0;
+ /* Various flags to the command. */
+ int cflags = 0, orig_cflags = 0, checked = 0, oautocont = -1;
+ FILE *oxtrerr = xtrerr, *newxtrerr = NULL;
+ /*
+ * Retrieve parameters for quick reference (they are unique
+ * to us so we can modify the structure if we want).
+ */
+ LinkList args = eparams->args;
+ LinkList redir = eparams->redir;
+ Wordcode varspc = eparams->varspc;
+ int type = eparams->type;
+
+ doneps4 = 0;
/*
* If assignment but no command get the status from variable
@@ -2854,7 +2887,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
/* Do prefork substitutions */
esprefork = (assign || isset(MAGICEQUALSUBST)) ? PREFORK_TYPESET : 0;
- if (args && htok)
+ if (args && eparams->htok)
prefork(args, esprefork, NULL);
if (type == WC_SIMPLE || type == WC_TYPESET) {
@@ -2999,7 +3032,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
/* Get the text associated with this command. */
if ((how & Z_ASYNC) ||
(!sfcontext && !sourcelevel && (jobbing || (how & Z_TIMED))))
- text = getjobtext(state->prog, beg);
+ text = getjobtext(state->prog, eparams->beg);
else
text = NULL;
@@ -3258,7 +3291,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
forked = 1;
}
- if ((esglob = !(cflags & BINF_NOGLOB)) && args && htok) {
+ if ((esglob = !(cflags & BINF_NOGLOB)) && args && eparams->htok) {
LinkList oargs = args;
globlist(args, 0);
args = oargs;
@@ -3572,7 +3605,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
}
if (type == WC_FUNCDEF) {
Eprog redir_prog;
- if (!redir && wc_code(*beg) == WC_REDIR) {
+ if (!redir && wc_code(*eparams->beg) == WC_REDIR) {
/*
* We're not using a redirection from the currently
* parsed environment, which is what we'd do for an
@@ -3582,7 +3615,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
struct estate s;
s.prog = state->prog;
- s.pc = beg;
+ s.pc = eparams->beg;
s.strs = state->prog->strs;
/*
@@ -3666,13 +3699,15 @@ execcmd(Estate state, int input, int output, int how, int last1)
} else {
/* It's a builtin */
LinkList assigns = (LinkList)0;
+ int postassigns = eparams->postassigns;
if (forked)
closem(FDT_INTERNAL);
if (postassigns) {
Wordcode opc = state->pc;
- state->pc = assignspc;
+ state->pc = eparams->assignspc;
assigns = newlinklist();
while (postassigns--) {
+ int htok;
wordcode ac = *state->pc++;
char *name = ecgetstr(state, EC_DUPTOK, &htok);
Asgment asg;
diff --git a/Src/zsh.h b/Src/zsh.h
index 79747d6..a5d4455 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -489,6 +489,7 @@ typedef struct complist *Complist;
typedef struct conddef *Conddef;
typedef struct dirsav *Dirsav;
typedef struct emulation_options *Emulation_options;
+typedef struct execcmd_params *Execcmd_params;
typedef struct features *Features;
typedef struct feature_enables *Feature_enables;
typedef struct funcstack *Funcstack;
@@ -1391,6 +1392,21 @@ struct builtin {
*/
#define BINF_ASSIGN (1<<19)
+/**
+ * Parameters passed to execcmd().
+ * These are not opaque --- they are also used by the pipeline manager.
+ */
+struct execcmd_params {
+ LinkList args; /* All command prefixes, arguments & options */
+ LinkList redir; /* Redirections */
+ Wordcode beg; /* The code at the start of the command */
+ Wordcode varspc; /* The code for assignment parsed as such */
+ Wordcode assignspc; /* The code for assignment parsed as typeset */
+ int type; /* The WC_* type of the command */
+ int postassigns; /* The number of assignspc assiguments */
+ int htok; /* tokens in parameter list */
+};
+
struct module {
struct hashnode node;
union {
diff --git a/Test/A01grammar.ztst b/Test/A01grammar.ztst
index 1ad73c5..0b1085c 100644
--- a/Test/A01grammar.ztst
+++ b/Test/A01grammar.ztst
@@ -757,12 +757,9 @@
>}
>Stuff here
-## This problem is hard to fix without significant changes to how
-## the shell forks for a pipeline.
-#
-# x=1
-# x=2 | echo $x
-# echo $x
-# 0:Assignment-only current shell commands in LHS of pipelin
-# >1
-# >1
+ x=1
+ x=2 | echo $x
+ echo $x
+0:Assignment-only current shell commands in LHS of pipelin
+>1
+>1
Messages sorted by:
Reverse Date,
Date,
Thread,
Author