Re: echo > * and EMFILE

[Bart Schaefer <schaefer@brasslantern.com>]

On Feb 13, 10:53am, Peter Stephenson wrote:
} Subject: Re: echo > * and EMFILE
}
} Stephane Chazelas wrote:
} > zsh obviously couldn't open that many files, which is normal,
} > 
} > But the problem here is that zsh didn't output any error
} > message.
} 
} There are lots and lots of unchecked system calls; it would be very
} useful to handle them better.

That may be true, but this is not a case of an unchecked system call.
movefd(), which actually makes the system call, does check the result
and do something appropriate, ending with returning -1 to propagate the
error to the calling scope.

Also, addfd() calls zerr() in some other circumstances, so I don't
think there's an inherent problem with calling it in this case too.

} Fixing them up as well as consistently handling the errors without,
} for example, file descriptor leaks needs a lot of work.

In fact I think the one place where addfd() does call zerr() is a file
descriptor leak, because zerr() has the side-effect of stopping the
command execution at that point and closemnodes() never gets called.

The next question is what is best from the user's point of view.
Suppose the example were something like

	print $SECONDS > *

If we call zerr() and closemnodes(), then an error message is printed
once but the command never actually executes, so the files that it was
possible to open are truncated to zero size.   If we call zwarn() and
not closemnodes(), then hundreds of error messages are printed, the
command goes ahead, some files get $SECONDS and some remain unchanged.
Which is preferable?

The patch for using zerr() would be something like this (line numbers
may be off, and the error remains a bit cryptic because all we know at
this point is the file descriptor number, not the file name):


Index: Src/exec.c
===================================================================
diff -c -r1.32 exec.c
--- Src/exec.c	1 Oct 2006 02:38:52 -0000	1.32
+++ Src/exec.c	13 Feb 2007 16:56:21 -0000
@@ -1632,11 +1674,24 @@
     } else {
 	if (mfds[fd1]->rflag != rflag) {
 	    zerr("file mode mismatch on fd %d", fd1);
+	    closemnodes(mfds);
 	    return;
 	}
 	if (mfds[fd1]->ct == 1) {	/* split the stream */
-	    mfds[fd1]->fds[0] = movefd(fd1);
-	    mfds[fd1]->fds[1] = movefd(fd2);
+	    int fdN = movefd(fd1);
+	    if (fdN < 0) {
+		zerr("multio failed for fd %d: %e", fd1, errno);
+		closemnodes(mfds);
+		return;
+	    }
+	    mfds[fd1]->fds[0] = fdN;
+	    fdN = movefd(fd2);
+	    if (fdN < 0) {
+		zerr("multio failed for fd %d: %e", fd2, errno);
+		closemnodes(mfds);
+		return;
+	    }
+	    mfds[fd1]->fds[1] = fdN;
 	    mpipe(pipes);
 	    mfds[fd1]->pipe = pipes[1 - rflag];
 	    redup(pipes[rflag], fd1);
@@ -1647,7 +1702,13 @@
 		int old = new - sizeof(int) * MULTIOUNIT;
 		mfds[fd1] = hrealloc((char *)mfds[fd1], old, new);
 	    }
-	    mfds[fd1]->fds[mfds[fd1]->ct++] = movefd(fd2);
+	    int fdN = movefd(fd2);
+	    if (fdN < 0) {
+		zerr("multio failed for fd %d: %e", fd2, errno);
+		closemnodes(mfds);
+		return;
+	    }
+	    mfds[fd1]->fds[mfds[fd1]->ct++] = fdN;
 	}
     }
     if (subsh_close >= 0 && fdtable[subsh_close] == FDT_UNUSED)