Re: 4.3.12-test-2

Re: 4.3.12-test-2

[Peter Stephenson]

On Fri, 21 Oct 2011 06:20:07 -0400
Phil Pennock <zsh-workers+phil.pennock@spodhuis.org> wrote:
> Between 4.3.12 and HEAD/4.3.12-test-2 I've started getting:
> 
> /home/phil/.zshenv:133: failed to close file descriptor 1: bad file descriptor
> /home/phil/.zshenv:136: failed to close file descriptor 1: bad file descriptor
> /home/phil/.zshenv:142: failed to close file descriptor 1: bad file descriptor
> /home/phil/.zshenv:146: failed to close file descriptor 1: bad file descriptor
> 
> Those correspond to function definitions of the form:
> 
> functions >&- have_cmd || function have_cmd { [[ -x =$1 ]] 2>/dev/null }

Hmm... I'm getting an error from closing stdout (just once), too.  That
doesn't seem right and needs looking into.  Possibly the error is
failing to close it, rather than the new error message (which is
deliberate if the shell fails to do an operation on file descriptors
you've explicitly asked for), so the real problem may not be new.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

Re: 4.3.12-test-2

[Peter Stephenson]

On Fri, 21 Oct 2011 12:08:48 +0100
Peter Stephenson <Peter.Stephenson@csr.com> wrote:
> > /home/phil/.zshenv:133: failed to close file descriptor 1: bad file descriptor
> 
> Hmm... I'm getting an error from closing stdout (just once), too.  That
> doesn't seem right and needs looking into.  Possibly the error is
> failing to close it, rather than the new error message (which is
> deliberate if the shell fails to do an operation on file descriptors
> you've explicitly asked for), so the real problem may not be new.

It looks like there isn't really a problem, apart from the error: we
were attempting to close an fd that had already been closed, which would
have caused a system error but not actually done any damage.  This stops
the error.  Could do with a test.

I got a bit worried about why we'd be moving the fd rather than just
closing it completely in the case of exec (which was showing up the same
error), but there's a special case later on for an exec that's just
doing redirections where we close the saved fd --- a bit baroque, but
presumably OK.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.204
diff -p -u -r1.204 exec.c
--- Src/exec.c	28 Aug 2011 16:38:28 -0000	1.204
+++ Src/exec.c	23 Oct 2011 17:15:05 -0000
@@ -2912,6 +2912,7 @@ execcmd(Estate state, int input, int out
 	    }
 	    addfd(forked, save, mfds, fn->fd1, fn->fd2, 1, fn->varid);
 	} else {
+	    int closed;
 	    if (fn->type != REDIR_HERESTR && xpandredir(fn, redir))
 		continue;
 	    if (errflag) {
@@ -3002,11 +3003,20 @@ execcmd(Estate state, int input, int out
 		 * Note we may attempt to close an fd beyond max_zsh_fd:
 		 * OK as long as we never look in fdtable for it.
  		 */
-		if (!forked && fn->fd1 < 10 && save[fn->fd1] == -2)
+		closed = 0;
+		if (!forked && fn->fd1 < 10 && save[fn->fd1] == -2) {
 		    save[fn->fd1] = movefd(fn->fd1);
+		    if (save[fn->fd1] >= 0) {
+			/*
+			 * The original fd is now closed, we don't need
+			 * to do it below.
+			 */
+			closed = 1;
+		    }
+		}
 		if (fn->fd1 < 10)
 		    closemn(mfds, fn->fd1);
-		if (zclose(fn->fd1) < 0) {
+		if (!closed && zclose(fn->fd1) < 0) {
 		    zwarn("failed to close file descriptor %d: %e",
 			  fn->fd1, errno);
 		}


-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: 4.3.12-test-2

[Peter Stephenson]

On Sun, 23 Oct 2011 18:19:18 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> Could do with a test.

This revealed that the existing tests actually expected errors on
closing stdout or stdin, which doesn't really make sense.

I think what confused me is I did expect errors with "print >&-", but we
don't get them (though we do with "exec >&-; print".  This seems to be a
deliberate special case.  We do an untested fwrite() followed by

	/* Testing EBADF special-cases >&- redirections */
	if ((fout != stdout) ? (fclose(fout) != 0) :
	    (fflush(fout) != 0 && errno != EBADF)) {
            zwarnnam(name, "write error: %e", errno);
            ret = 1;
	}

This looks pretty flaky to me but there must be some history behind it.

Index: Test/A04redirect.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/A04redirect.ztst,v
retrieving revision 1.19
diff -p -u -r1.19 A04redirect.ztst
--- Test/A04redirect.ztst	27 Jul 2011 19:14:18 -0000	1.19
+++ Test/A04redirect.ztst	23 Oct 2011 17:41:57 -0000
@@ -156,11 +156,14 @@
   read foo <&-)
 1:'<&-' redirection
 ?(eval):1: failed to close file descriptor 3: bad file descriptor
-?(eval):2: failed to close file descriptor 0: bad file descriptor
 
   print foo >&-
 0:'>&-' redirection
-?(eval):1: failed to close file descriptor 1: bad file descriptor
+
+  (exec >&-
+  print foo)
+0:'>&-' with attempt to use closed fd
+?(eval):2: write error: bad file descriptor
 
   fn() { local foo; read foo; print $foo; }
   coproc fn

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/