Re: PATCH: functions with redirections

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Thu, 02 Oct 2014 09:35:09 +0100
Peter Stephenson <p.stephenson@xxxxxxxxxxx> wrote:
> > torch% cat /tmp/foo
> > foo() { echo foo } >&3
> > torch% zcompile -k /tmp/foo
> > torch% autoload foo
> > torch% FPATH=/tmp foo      
> > foo
> > 
> > Oops, the redirection didn't get applied.
> 
> Not sure how that could happen --- there must be some kludge to run the
> function the first time.

That's correct:  we use a very simple execution environment to execute a
function that's just been autoloaded.  This isn't actually related to
dumps --- we have the same problem with a normal ksh autoload.

The fix is to promote the point where we actually load (but don't
execute) the function.  We do this for all autoloads to find out if
there are redirections.  Having done that we are in a position to
extract the redirections as usual.

The normal zsh case wouldn't have redirections at that point because the
syntax doesn't allow it.  The kludged zsh case with an explicit call to
the function (the one I originally tested) works because in that case
all the normal function definition code is executed; it's not a special
case as far as redirection code is concerned.

We don't currently set errflag when failing to load a function (unless
it happens deeper than I've looked), so we still don't.  There'a at
least some sign this is deliberate (warnings rather than errors).

Moving the load up might have some very minor side effect on some error
case where previously we wouldn't have defined the function but now do.

pws

diff --git a/Src/exec.c b/Src/exec.c
index 10f71da..a5452e5 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2789,13 +2789,6 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	    errflag = 1;
     }
 
-    if (errflag) {
-	lastval = 1;
-	if (oautocont >= 0)
-	    opts[AUTOCONTINUE] = oautocont;
-	return;
-    }
-
     if (type == WC_FUNCDEF) {
 	/*
 	 * The first word of a function definition is a list of
@@ -2809,8 +2802,24 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	    /* Nonymous, don't do redirections here */
 	    redir = NULL;
 	}
-    } else if (is_shfunc) {
-	Shfunc shf = (Shfunc)hn;
+    } else if (is_shfunc || type == WC_AUTOFN) {
+	Shfunc shf;
+	if (is_shfunc)
+	    shf = (Shfunc)hn;
+	else {
+	    shf = loadautofn(state->prog->shf, 1, 0);
+	    if (shf)
+		state->prog->shf = shf;
+	    else {
+		/*
+		 * This doesn't set errflag, so just return now.
+		 */
+		lastval = 1;
+		if (oautocont >= 0)
+		    opts[AUTOCONTINUE] = oautocont;
+		return;
+	    }
+	}
 	/*
 	 * A function definition may have a list of additional
 	 * redirections to apply, so retrieve it.
@@ -2832,6 +2841,13 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	}
     }
 
+    if (errflag) {
+	lastval = 1;
+	if (oautocont >= 0)
+	    opts[AUTOCONTINUE] = oautocont;
+	return;
+    }
+
     if (type == WC_SIMPLE && !nullexec) {
 	char *s;
 	char trycd = (isset(AUTOCD) && isset(SHINSTDIN) &&
@@ -3306,10 +3322,18 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		redir_prog = NULL;
 	    
 	    lastval = execfuncdef(state, redir_prog);
-	} else if (type >= WC_CURSH) {
+	} 
+	else if (type >= WC_CURSH) {
 	    if (last1 == 1)
 		do_exec = 1;
-	    lastval = (execfuncs[type - WC_CURSH])(state, do_exec);
+	    if (type == WC_AUTOFN) {
+		/*
+		 * We pre-loaded this to get any redirs.
+		 * So we execuate a simplified function here.
+		 */
+		lastval =  execautofn_basic(state, do_exec);
+	    } else
+		lastval = (execfuncs[type - WC_CURSH])(state, do_exec);
 	} else if (is_builtin || is_shfunc) {
 	    LinkList restorelist = 0, removelist = 0;
 	    /* builtin or shell function */
@@ -4540,21 +4564,28 @@ execshfunc(Shfunc shf, LinkList args)
 	deletefilelist(last_file_list, 0);
 }
 
-/* Function to execute the special type of command that represents an *
- * autoloaded shell function.  The command structure tells us which   *
- * function it is.  This function is actually called as part of the   *
- * execution of the autoloaded function itself, so when the function  *
- * has been autoloaded, its list is just run with no frills.          */
+/*
+ * Function to execute the special type of command that represents an
+ * autoloaded shell function.  The command structure tells us which
+ * function it is.  This function is actually called as part of the
+ * execution of the autoloaded function itself, so when the function
+ * has been autoloaded, its list is just run with no frills.
+ *
+ * There are two cases because if we are doing all-singing, all-dancing
+ * non-simple code we load the shell function early in execcmd() (the
+ * action also present in the non-basic version) to check if
+ * there are redirections that need to be handled at that point.
+ * Then we call execautofn_basic() to do the rest.
+ */
 
 /**/
 static int
-execautofn(Estate state, UNUSED(int do_exec))
+execautofn_basic(Estate state, UNUSED(int do_exec))
 {
     Shfunc shf;
     char *oldscriptname, *oldscriptfilename;
 
-    if (!(shf = loadautofn(state->prog->shf, 1, 0)))
-	return 1;
+    shf = state->prog->shf;
 
     /*
      * Probably we didn't know the filename where this function was
@@ -4574,6 +4605,19 @@ execautofn(Estate state, UNUSED(int do_exec))
 }
 
 /**/
+static int
+execautofn(Estate state, UNUSED(int do_exec))
+{
+    Shfunc shf;
+
+    if (!(shf = loadautofn(state->prog->shf, 1, 0)))
+	return 1;
+
+    state->prog->shf = shf;
+    return execautofn_basic(state, 0);
+}
+
+/**/
 Shfunc
 loadautofn(Shfunc shf, int fksh, int autol)
 {
diff --git a/Test/A05execution.ztst b/Test/A05execution.ztst
index df6d7bc..8d256ff 100644
--- a/Test/A05execution.ztst
+++ b/Test/A05execution.ztst
@@ -216,3 +216,16 @@ F:This similar test was triggering a reproducible failure with pipestatus.
 >done
 F:This test checks for a file descriptor leak that could cause the left
 F:side of a pipe to block on write after the right side has exited
+
+  print "autoload_redir() { print Autoloaded ksh style; } >autoload.log" >autoload_redir
+  autoload -Uk autoload_redir
+  autoload_redir
+  print No output yet
+  cat autoload.log
+  functions autoload_redir
+0:
+>No output yet
+>Autoloaded ksh style
+>autoload_redir () {
+>	print Autoloaded ksh style
+>} > autoload.log