Re: PATCH: allocating a new file descriptor

[Peter Stephenson <pws@xxxxxxx>]

Bart Schaefer wrote:
> On Apr 14, 10:49am, Peter Stephenson wrote:
> } Subject: Re: PATCH: allocating a new file descriptor
> }
> } I can add documentation to point this out if you think it would be
> } useful.
> 
> Possibly so.

I've tried to improve it.

> } It would also be possible to make NO_CLOBBER check that in {myfd}>stuff,
> } $myfd doesn't already point to an fd with an FDT_EXTERNAL flag.  That
> } would prevent overwriting.  I quite like that idea.
> 
> I do, too.

I've done this, and in the process handled some other error cases
better, though it's meant the code is a little more verbose.  It now
doesn't create the file on {myfd}>file if myfd is readonly and I picked
up some possible leaks on errors.

> } For more complicated cases you might need to set myfd to 0 first
> 
> Or (more appropriately, I would think) unset it, no?

Probably, I was thinking of the case where you'd just declared it as an
integer, but in practice the effect is the same.

> It might be useful to add a note somewhere (the "Expansion" section?)
> about the ordering of parameter expansion relative to redirections.

I couldn't think of anywhere obvious in that section, but I've made it
more explicit in the redirection section.

> But this causes the shell to hang forever:
> 
>     print foo {myfd}>>(tr a-z A-Z) >&$myfd

The fix to make the shell wait until the process exited (to make this
synchronous when used as a pipe for stdout or stderr) was coming into
effect, but shouldn't have since the fd stays valid until explicitly
closed.  This fixes it.

Index: Doc/Zsh/redirect.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/redirect.yo,v
retrieving revision 1.8
diff -u -r1.8 redirect.yo
--- Doc/Zsh/redirect.yo	12 Apr 2005 15:11:11 -0000	1.8
+++ Doc/Zsh/redirect.yo	14 Apr 2005 16:04:02 -0000
@@ -174,6 +174,27 @@
 descriptor using tt(<&$)var(param) or tt(>&$)var(param) if var(param) is
 readonly.
 
+If the option tt(CLOBBER) is unset, it is an error to open a file
+descriptor using a parameter that is already set to an open file descriptor
+previously allocated by this mechanism.  Unsetting the parameter before
+using it for allocating a file descriptor avoids the error.
+
+Note that this mechanism merely allocates or closes a file descriptor; it
+does not perform any redirections from or to it.  It is usually convenient
+to allocate a file descriptor prior to use as an argument to tt(exec).  The
+following shows a typical sequence of allocation, use, and closing of a
+file descriptor:
+
+example(integer myfd
+exec {myfd}>~/logs/mylogfile.txt
+print This is a log message. >&$myfd
+exec {myfd}>&-)
+
+Note that the expansion of the variable in the expression tt(>&$myfd)
+occurs at the point the redirection is opened.  This is after the expansion
+of command arguments and after any redirections to the left on the command
+line have been processed.
+
 The `tt(|&)' command separator described in
 ifzman(em(Simple Commands & Pipelines) in zmanref(zshmisc))\
 ifnzman(noderef(Simple Commands & Pipelines))
@@ -230,6 +251,10 @@
 
 is equivalent to `tt(cat foo fubar | sort)'.
 
+Expansion of the redirection argument occurs at the point the redirection
+is opened, at the point described above for the expansion of the variable
+in tt(>&$myfd).
+
 Note that a pipe is an implicit redirection; thus
 
 example(cat bar | sort <foo)
Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.87
diff -u -r1.87 exec.c
--- Src/exec.c	12 Apr 2005 15:11:11 -0000	1.87
+++ Src/exec.c	14 Apr 2005 16:04:03 -0000
@@ -1405,6 +1405,41 @@
     }
 }
 
+/* Check that we can use a parameter for allocating a file descriptor. */
+
+static int
+checkclobberparam(struct redir *f)
+{
+    struct value vbuf;
+    Value v;
+    char *s = f->varid;
+    int fd;
+
+    if (!s)
+	return 1;
+
+    if (!(v = getvalue(&vbuf, &s, 0)))
+	return 1;
+
+    if (v->pm->flags & PM_READONLY) {
+	zwarn("can't allocate file descriptor to readonly parameter %s",
+	      f->varid, 0);
+	/* don't flag a system error for this */
+	errno = 0;
+	return 0;
+    }
+
+    if (!isset(CLOBBER) && (fd = (int)getintvalue(v)) &&
+	fd <= max_zsh_fd && fdtable[fd] == FDT_EXTERNAL) {
+	zwarn("can't clobber parameter %s containing file descriptor %d",
+	     f->varid, fd);
+	/* don't flag a system error for this */
+	errno = 0;
+	return 0;
+    }
+    return 1;
+}
+
 /* Open a file for writing redirection */
 
 /**/
@@ -2239,17 +2274,22 @@
     /* Do io redirections */
     while (redir && nonempty(redir)) {
 	fn = (Redir) ugetnode(redir);
+
 	DPUTS(fn->type == REDIR_HEREDOC || fn->type == REDIR_HEREDOCDASH,
 	      "BUG: unexpanded here document");
 	if (fn->type == REDIR_INPIPE) {
-	    if (fn->fd2 == -1) {
+	    if (!checkclobberparam(fn) || fn->fd2 == -1) {
+		if (fn->fd2 != -1)
+		    zclose(fn->fd2);
 		closemnodes(mfds);
 		fixfds(save);
 		execerr();
 	    }
 	    addfd(forked, save, mfds, fn->fd1, fn->fd2, 0, fn->varid);
 	} else if (fn->type == REDIR_OUTPIPE) {
-	    if (fn->fd2 == -1) {
+	    if (!checkclobberparam(fn) || fn->fd2 == -1) {
+		if (fn->fd2 != -1)
+		    zclose(fn->fd2);
 		closemnodes(mfds);
 		fixfds(save);
 		execerr();
@@ -2271,11 +2311,14 @@
 		continue;
 	    switch(fn->type) {
 	    case REDIR_HERESTR:
-		fil = getherestr(fn);
+		if (!checkclobberparam(fn))
+		    fil = -1;
+		else
+		    fil = getherestr(fn);
 		if (fil == -1) {
 		    closemnodes(mfds);
 		    fixfds(save);
-		    if (errno != EINTR)
+		    if (errno && errno != EINTR)
 			zwarn("%e", NULL, errno);
 		    execerr();
 		}
@@ -2283,7 +2326,9 @@
 		break;
 	    case REDIR_READ:
 	    case REDIR_READWRITE:
-		if (fn->type == REDIR_READ)
+		if (!checkclobberparam(fn))
+		    fil = -1;
+		else if (fn->type == REDIR_READ)
 		    fil = open(unmeta(fn->name), O_RDONLY | O_NOCTTY);
 		else
 		    fil = open(unmeta(fn->name),
@@ -2335,7 +2380,9 @@
 	    case REDIR_MERGEOUT:
 		if (fn->fd2 < 10)
 		    closemn(mfds, fn->fd2);
-		if (fn->fd2 > 9 &&
+		if (!checkclobberparam(fn))
+		    fil = -1;
+		else if (fn->fd2 > 9 &&
 		    ((fdtable[fn->fd2] != FDT_UNUSED &&
 		      fdtable[fn->fd2] != FDT_EXTERNAL) ||
 		     fn->fd2 == coprocin ||
@@ -2355,14 +2402,18 @@
 		    fixfds(save);
 		    if (fn->fd2 != -2)
 		    	sprintf(fdstr, "%d", fn->fd2);
-		    zwarn("%s: %e", fn->fd2 == -2 ? "coprocess" : fdstr, errno);
+		    if (errno)
+			zwarn("%s: %e", fn->fd2 == -2 ? "coprocess" : fdstr,
+			      errno);
 		    execerr();
 		}
 		addfd(forked, save, mfds, fn->fd1, fil,
 		      fn->type == REDIR_MERGEOUT, fn->varid);
 		break;
 	    default:
-		if (IS_APPEND_REDIR(fn->type))
+		if (!checkclobberparam(fn))
+		    fil = -1;
+		else if (IS_APPEND_REDIR(fn->type))
 		    fil = open(unmeta(fn->name),
 			       (unset(CLOBBER) && !IS_CLOBBER_REDIR(fn->type)) ?
 			       O_WRONLY | O_APPEND | O_NOCTTY :
@@ -2378,7 +2429,7 @@
 			close(fil);
 		    closemnodes(mfds);
 		    fixfds(save);
-		    if (errno != EINTR)
+		    if (errno && errno != EINTR)
 			zwarn("%e: %s", fn->name, errno);
 		    execerr();
 		}
@@ -2387,7 +2438,10 @@
 		    addfd(forked, save, mfds, 2, dfil, 1, NULL);
 		break;
 	    }
+	    /* May be error in addfd due to setting parameter. */
 	    if (errflag) {
+		closemnodes(mfds);
+		fixfds(save);
 		execerr();
 	    }
 	}
@@ -3234,6 +3288,9 @@
  * If the second argument is 1, this is part of
  * an "exec < <(...)" or "exec > >(...)" and we shouldn't
  * wait for the job to finish before continuing.
+ * Likewise, we shouldn't wait if we are opening the file
+ * descriptor using the {fd}>>(...) notation since it stays
+ * valid for subsequent commands.
  */
 
 /**/
@@ -3249,7 +3306,7 @@
 	f = (Redir) getdata(n);
 	if (f->type == REDIR_OUTPIPE || f->type == REDIR_INPIPE) {
 	    str = f->name;
-	    f->fd2 = getpipe(str, nullexec);
+	    f->fd2 = getpipe(str, nullexec || f->varid);
 	}
     }
 }
Index: Test/A04redirect.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/A04redirect.ztst,v
retrieving revision 1.7
diff -u -r1.7 A04redirect.ztst
--- Test/A04redirect.ztst	12 Apr 2005 17:40:06 -0000	1.7
+++ Test/A04redirect.ztst	14 Apr 2005 16:04:03 -0000
@@ -248,6 +248,11 @@
 >Examining contents of logfile...
 >This is my logfile.
 
+  setopt noclobber
+  exec {myfd}>logfile2
+1q:NO_CLOBBER prevents overwriting parameter with allocated fd
+?(eval):2: can't clobber parameter myfd containing file descriptor $myfd
+
   exec {myfd}>&-
   print This message should disappear >&$myfd
 1q:Closing file descriptor using brace syntax
@@ -256,7 +261,7 @@
   typeset -r myfd
   echo This should not appear {myfd}>nologfile
 1:Error opening file descriptor using readonly variable
-?(eval):2: read-only variable: myfd
+?(eval):2: can't allocate file descriptor to readonly parameter myfd
 
   typeset +r myfd
   exec {myfd}>newlogfile


-- 
Peter Stephenson <pws@xxxxxxx>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

**********************************************************************