PATCH: coprocess descriptor and error messages

PATCH: coprocess descriptor and error messages

[Bart Schaefer]

This seems somehow wrong to me ... starting from "zsh -f":

torch% print -p ; echo $?
print: bad file number: -1
1
torch% read -p ; echo $?
1

Seems to me that both commands should either fail silently, or print a
more specific error.  Patch below does the former.  (Line numbers may
be off.)  I won't commit until I see commentary.

Index: Src/builtin.c
===================================================================
RCS file: /extra/cvsroot/zsh/zsh-4.0/Src/builtin.c,v
retrieving revision 1.48
diff -c -r1.48 builtin.c
--- builtin.c	17 Apr 2009 18:57:22 -0000	1.48
+++ builtin.c	17 Sep 2010 05:02:15 -0000
@@ -3677,13 +3720,17 @@
     if (OPT_HASARG(ops,'u') || OPT_ISSET(ops,'p')) {
 	int fd;
 
-	if (OPT_ISSET(ops, 'p'))
+	if (OPT_ISSET(ops, 'p')) {
 	    fd = coprocout;
-	else {
+	    if (fd < 0)
+		return 1;
+	} else {
 	    char *argptr = OPT_ARG(ops,'u'), *eptr;
 	    /* Handle undocumented feature that -up worked */
 	    if (!strcmp(argptr, "p")) {
 		fd = coprocout;
+		if (fd < 0)
+		    return 1;
 	    } else {
 		fd = (int)zstrtol(argptr, &eptr, 10);
 		if (*eptr) {


-- 

Re: PATCH: coprocess descriptor and error messages

[Peter Stephenson]

On Thu, 16 Sep 2010 22:04:42 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> This seems somehow wrong to me ... starting from "zsh -f":
> 
> torch% print -p ; echo $?
> print: bad file number: -1
> 1
> torch% read -p ; echo $?
> 1
> 
> Seems to me that both commands should either fail silently, or print a
> more specific error.  Patch below does the former.  (Line numbers may
> be off.)  I won't commit until I see commentary.

The question is whether it's useful to be able to test whether the
coprocess is open at the same time as attempting to read or write.  If so,
doing it silently is reasonable.  Otherwise it might be less mysterious to
print "coprocess not open", or something like that, in both cases.

-- 
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

Re: PATCH: coprocess descriptor and error messages

[Bart Schaefer]

On Sep 17,  9:46am, Peter Stephenson wrote:
}
} The question is whether it's useful to be able to test whether the
} coprocess is open at the same time as attempting to read or write.

Is there another obvious way to test whether the coprocess is open?

Aside:  Should an interactive shell ever exit on SIGPIPE?  Or maybe
a better question is, should a print to the coprocess ever cause an
un-trapped SIGPIPE?

schaefer[713] Src/zsh -f
torch% coproc repeat 8 read -E
[1] 31339
torch% repeat 10 { print -p; read -p }
schaefer[714] echo $?
141

} If so, doing it silently is reasonable. Otherwise it might be less
} mysterious to print "coprocess not open", or something like that,
} in both cases.

In looking further, the coprocess descriptor is reset to -1 only (and
immediately) by "read -p" detecting EOF on the coprocess output.  So
in the "read" case, the invalid descriptor has always served as an
EOF flag.  What should happen if the parent attempts to read after
an EOF has been seen?  I would tend to think it should just get EOF
again, not produce a diagnostic, which is the current behavior. 

However, ksh (at least pdksh) disagrees with me:

$ print -p
ksh: print: -p: no coprocess
$ read |&
[1] 31399
$ print -p
$ print -p
ksh: print: -p: no coprocess
[1] + Done                 read 
$ read -p
$ read -p
ksh: read: -p: no coprocess
$ print -p
ksh: print: -p: no coprocess
$ 

Note, however, that it does NOT exit with SIGPIPE on "print -p".

Re: PATCH: coprocess descriptor and error messages

[Bart Schaefer]

On Sep 17,  7:41am, Bart Schaefer wrote:
}
} Aside:  Should an interactive shell ever exit on SIGPIPE?  Or maybe
} a better question is, should a print to the coprocess ever cause an
} un-trapped SIGPIPE?

Experimenting with pdksh seems to indicate that the equivalent of
zsh's coprocout descriptor is closed in the child-exited handler
when the coprocess terminates.  I don't recall enough about (and
didn't yet delve back into) the zsh job table to know if we have
enough information to identify the coprocess job at that point.

The coprocin descriptor remains open until EOF, as in zsh.

} In looking further, the coprocess descriptor is reset to -1 only (and
} immediately) by "read -p" detecting EOF on the coprocess output.  So
} in the "read" case, the invalid descriptor has always served as an
} EOF flag.  What should happen if the parent attempts to read after
} an EOF has been seen?  I would tend to think it should just get EOF
} again, not produce a diagnostic, which is the current behavior. 
} 
} However, ksh (at least pdksh) disagrees with me:

Here's a patch that duplicates the pdksh behavior, except for the part
about exiting on SIGPIPE if an extra print occurs before all output
has been read.  Apply this *instead* of the previous patch.

Index: builtin.c
===================================================================
RCS file: /extra/cvsroot/zsh/zsh-4.0/Src/builtin.c,v
retrieving revision 1.48
diff -c -r1.48 builtin.c
--- builtin.c	17 Apr 2009 18:57:22 -0000	1.48
+++ builtin.c	17 Sep 2010 15:59:06 -0000
@@ -3677,13 +3720,21 @@
     if (OPT_HASARG(ops,'u') || OPT_ISSET(ops,'p')) {
 	int fd;
 
-	if (OPT_ISSET(ops, 'p'))
+	if (OPT_ISSET(ops, 'p')) {
 	    fd = coprocout;
-	else {
+	    if (fd < 0) {
+		zwarnnam(name, "-p: no coprocess");
+		return 1;
+	    }
+	} else {
 	    char *argptr = OPT_ARG(ops,'u'), *eptr;
 	    /* Handle undocumented feature that -up worked */
 	    if (!strcmp(argptr, "p")) {
 		fd = coprocout;
+		if (fd < 0) {
+		    zwarnnam(name, "-p: no coprocess");
+		    return 1;
+		}
 	    } else {
 		fd = (int)zstrtol(argptr, &eptr, 10);
 		if (*eptr) {
@@ -5038,6 +5099,10 @@
 	/* The old code handled -up, but that was never documented. Still...*/
 	if (!strcmp(argptr, "p")) {
 	    readfd = coprocin;
+	    if (readfd < 0) {
+		zwarnnam(name, "-p: no coprocess");
+		return 1;
+	    }
 	} else {
 	    readfd = (int)zstrtol(argptr, &eptr, 10);
 	    if (*eptr) {
@@ -5052,6 +5117,10 @@
 	izle = 0;
     } else if (OPT_ISSET(ops,'p')) {
 	readfd = coprocin;
+	if (readfd < 0) {
+	    zwarnnam(name, "-p: no coprocess");
+	    return 1;
+	}
 	izle = 0;
     } else
 	readfd = izle = 0;