warning about closing an already closed file descriptor

warning about closing an already closed file descriptor

[Kamil Dudka]

zsh prints a warning if an already closed file descriptor is to be closed.  
While this is technically correct, the other shells I tried (ksh and bash)
do not print any such warning:

$ bash -c '(true <&-) <&-'
$ ksh -c '(true <&-) <&-'
$ zsh -c '(true <&-) <&-'
zsh:1: failed to close file descriptor 0: bad file descriptor

The warning was introduced by the following commit:
http://sourceforge.net/p/zsh/code/ci/45913f43

... and it causes problems when porting legacy scripts to a newer version
of zsh.  Is there any way to suppress the warning?

If not, how can one achieve compatibility with other shells (including
older versions of zsh)?

Kamil

Re: warning about closing an already closed file descriptor

[Peter Stephenson]

On Mon, 19 Jan 2015 14:33:25 +0100
Kamil Dudka <kdudka@redhat.com> wrote:
> zsh prints a warning if an already closed file descriptor is to be closed.  
> While this is technically correct, the other shells I tried (ksh and bash)
> do not print any such warning:
> 
> $ bash -c '(true <&-) <&-'
> $ ksh -c '(true <&-) <&-'
> $ zsh -c '(true <&-) <&-'
> zsh:1: failed to close file descriptor 0: bad file descriptor
> 
> The warning was introduced by the following commit:
> http://sourceforge.net/p/zsh/code/ci/45913f43
> 
> ... and it causes problems when porting legacy scripts to a newer version
> of zsh.  Is there any way to suppress the warning?
> 
> If not, how can one achieve compatibility with other shells (including
> older versions of zsh)?

The problem that change fixed wasn't really intended to have a visible
effect, apart from closing f.d.s the shell didn't know about.  If it's
more useful without the message than with it, it can be removed.  The
shell doesn't make it particularly convenient to test first, either (you
can test something that isn't quite the point by means that aren't quite
intended for it).

Does anyone want to own up to using the message for debugging?

We can special case the {varid}<&- syntax --- that might be useful since
it implies the user was previously in direct control of the f.d. so is
ikely to be interested in an error if closing it failed.

pws

Re: warning about closing an already closed file descriptor

[Peter Stephenson]

On Mon, 19 Jan 2015 14:59:35 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> The problem that change fixed wasn't really intended to have a visible
> effect, apart from closing f.d.s the shell didn't know about.  If it's
> more useful without the message than with it, it can be removed.
>...
> We can special case the {varid}<&- syntax --- that might be useful since
> it implies the user was previously in direct control of the f.d. so is
> ikely to be interested in an error if closing it failed.

This easy patch does that.  Is anyone other than Kamil interested enough
to comment?

pws

diff --git a/Src/exec.c b/Src/exec.c
index f42fb2b..3b0e936 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3203,7 +3203,12 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		}
 		if (fn->fd1 < 10)
 		    closemn(mfds, fn->fd1, REDIR_CLOSE);
-		if (!closed && zclose(fn->fd1) < 0) {
+		/*
+		 * Only report failures to close file descriptors
+		 * if they're under user control as we don't know
+		 * what the previous status of others was.
+		 */
+		if (!closed && zclose(fn->fd1) < 0 && fn->varid) {
 		    zwarn("failed to close file descriptor %d: %e",
 			  fn->fd1, errno);
 		}
diff --git a/Test/A04redirect.ztst b/Test/A04redirect.ztst
index a39ce46..cb67788 100644
--- a/Test/A04redirect.ztst
+++ b/Test/A04redirect.ztst
@@ -152,11 +152,13 @@
 >hello
 >goodbye
 
-  ({ exec 3<&- } 2>/dev/null
-  exec 3<&-
-  read foo <&-)
+  (exec {varid}<&0
+  exec {varid}<&-
+  print About to close a second time >&2
+  read {varid}<&-)
 1:'<&-' redirection
-*?\(eval\):*: failed to close file descriptor 3:*
+?About to close a second time
+*?\(eval\):*: failed to close file descriptor *
 
   print foo >&-
 0:'>&-' redirection

Re: warning about closing an already closed file descriptor

[Kamil Dudka]

On Monday 19 January 2015 17:28:50 Peter Stephenson wrote:
> On Mon, 19 Jan 2015 14:59:35 +0000
> 
> Peter Stephenson <p.stephenson@samsung.com> wrote:
> > The problem that change fixed wasn't really intended to have a visible
> > effect, apart from closing f.d.s the shell didn't know about.  If it's
> > more useful without the message than with it, it can be removed.
> >
> >...
> >
> > We can special case the {varid}<&- syntax --- that might be useful since
> > it implies the user was previously in direct control of the f.d. so is
> > ikely to be interested in an error if closing it failed.
> 
> This easy patch does that.  Is anyone other than Kamil interested enough
> to comment?
> 
> pws
> 
> diff --git a/Src/exec.c b/Src/exec.c
> index f42fb2b..3b0e936 100644
> --- a/Src/exec.c
> +++ b/Src/exec.c
> @@ -3203,7 +3203,12 @@ execcmd(Estate state, int input, int output, int how,
> int last1) }
>  		if (fn->fd1 < 10)
>  		    closemn(mfds, fn->fd1, REDIR_CLOSE);
> -		if (!closed && zclose(fn->fd1) < 0) {
> +		/*
> +		 * Only report failures to close file descriptors
> +		 * if they're under user control as we don't know
> +		 * what the previous status of others was.
> +		 */
> +		if (!closed && zclose(fn->fd1) < 0 && fn->varid) {
>  		    zwarn("failed to close file descriptor %d: %e",
>  			  fn->fd1, errno);
>  		}

Looks reasonable and fixes the problem for me.  Thanks for the patch!

Kamil

> diff --git a/Test/A04redirect.ztst b/Test/A04redirect.ztst
> index a39ce46..cb67788 100644
> --- a/Test/A04redirect.ztst
> +++ b/Test/A04redirect.ztst
> @@ -152,11 +152,13 @@
> 
>  >hello
>  >goodbye
> 
> -  ({ exec 3<&- } 2>/dev/null
> -  exec 3<&-
> -  read foo <&-)
> +  (exec {varid}<&0
> +  exec {varid}<&-
> +  print About to close a second time >&2
> +  read {varid}<&-)
>  1:'<&-' redirection
> -*?\(eval\):*: failed to close file descriptor 3:*
> +?About to close a second time
> +*?\(eval\):*: failed to close file descriptor *
> 
>    print foo >&-
>  0:'>&-' redirection

Re: warning about closing an already closed file descriptor

[Roman Neuhauser]

# p.stephenson@samsung.com / 2015-01-19 17:28:50 +0000:
> On Mon, 19 Jan 2015 14:59:35 +0000
> Peter Stephenson <p.stephenson@samsung.com> wrote:
> > The problem that change fixed wasn't really intended to have a visible
> > effect, apart from closing f.d.s the shell didn't know about.  If it's
> > more useful without the message than with it, it can be removed.
> >...
> > We can special case the {varid}<&- syntax --- that might be useful since
> > it implies the user was previously in direct control of the f.d. so is
> > ikely to be interested in an error if closing it failed.
> 
> This easy patch does that.  Is anyone other than Kamil interested enough
> to comment?

> diff --git a/Test/A04redirect.ztst b/Test/A04redirect.ztst
> index a39ce46..cb67788 100644
> --- a/Test/A04redirect.ztst
> +++ b/Test/A04redirect.ztst
> @@ -152,11 +152,13 @@
>  >hello
>  >goodbye
>  
> -  ({ exec 3<&- } 2>/dev/null
> -  exec 3<&-
> -  read foo <&-)
> +  (exec {varid}<&0
> +  exec {varid}<&-
> +  print About to close a second time >&2
> +  read {varid}<&-)

sorry if i'm talking out of my ass, but does this mean that
the non-{varid} syntax is now without a test?

>  1:'<&-' redirection
> -*?\(eval\):*: failed to close file descriptor 3:*
> +?About to close a second time
> +*?\(eval\):*: failed to close file descriptor *
>  
>    print foo >&-
>  0:'>&-' redirection

-- 
roman

Re: warning about closing an already closed file descriptor

[Peter Stephenson]

On Tue, 20 Jan 2015 12:02:03 +0100
Roman Neuhauser <neuhauser@sigpipe.cz> wrote:
> > -  ({ exec 3<&- } 2>/dev/null
> > -  exec 3<&-
> > -  read foo <&-)
> > +  (exec {varid}<&0
> > +  exec {varid}<&-
> > +  print About to close a second time >&2
> > +  read {varid}<&-)
> 
> sorry if i'm talking out of my ass, but does this mean that
> the non-{varid} syntax is now without a test?

For closing fd's, yes.  There's no output to test --- though there is a
status, so we could still test that.  And I suppose the lack of an error
message is itself a test...

pws

diff --git a/Test/A04redirect.ztst b/Test/A04redirect.ztst
index cb67788..13f1f7c 100644
--- a/Test/A04redirect.ztst
+++ b/Test/A04redirect.ztst
@@ -152,11 +152,16 @@
 >hello
 >goodbye
 
+  ({exec 3<&- } 2>/dev/null
+   exec 3<&-
+   read foo <&-)
+1:'<&-' redirection with numeric fd (no error message on failure)
+
   (exec {varid}<&0
   exec {varid}<&-
   print About to close a second time >&2
   read {varid}<&-)
-1:'<&-' redirection
+1:'<&-' redirection with fd in variable (error message on failure)
 ?About to close a second time
 *?\(eval\):*: failed to close file descriptor *
 

Re: warning about closing an already closed file descriptor

[Roman Neuhauser]

# p.stephenson@samsung.com / 2015-01-20 11:39:43 +0000:
> On Tue, 20 Jan 2015 12:02:03 +0100
> Roman Neuhauser <neuhauser@sigpipe.cz> wrote:
> > > -  ({ exec 3<&- } 2>/dev/null
> > > -  exec 3<&-
> > > -  read foo <&-)
> > > +  (exec {varid}<&0
> > > +  exec {varid}<&-
> > > +  print About to close a second time >&2
> > > +  read {varid}<&-)
> > 
> > sorry if i'm talking out of my ass, but does this mean that
> > the non-{varid} syntax is now without a test?
> 
> For closing fd's, yes.  There's no output to test --- though there is a
> status, so we could still test that.  And I suppose the lack of an error
> message is itself a test...

yup, testing that stderr is actually quiet was my concern.  thanks a lot!

-- 
roman