$(<nofile) doesn't set $? to non-zero

$(<nofile) doesn't set $? to non-zero

[Stephane Chazelas]

$ zsh -c 'ar=$(<file); echo "$?"'
zsh:1: no such file or directory: file
0

That's different from other shells (ksh93, bash, mksh) that
return 1, and is also inconsistent with the cases where $(<) is
not recognised as that operator and runs the NULLCMD instead:

$ zsh -c 'var=$(<file >/dev/null); echo "$?"'
zsh:1: no such file or directory: file
1

Note that all shells are silent and return 0 for var=$(</etc)
var=$(</dev/mem) or any file that can be opened but not read
successfully, which IMO is not ideal.

-- 
Stephane

Re: $(<nofile) doesn't set $? to non-zero

[Peter Stephenson]

On Wed, 14 Mar 2018 10:32:54 +0000
Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> $ zsh -c 'ar=$(<file); echo "$?"'
> zsh:1: no such file or directory: file
> 0
> 
> That's different from other shells (ksh93, bash, mksh) that
> return 1, and is also inconsistent with the cases where $(<) is
> not recognised as that operator and runs the NULLCMD instead:

We set lastval = cmdoutval if it's an ordinary command substitution, so it's
easy to make this consistent with that behaviour.  Doing so seems
unproblematic.

pws

diff --git a/Src/exec.c b/Src/exec.c
index e5c6455..299b816 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4514,6 +4514,7 @@ getoutput(char *cmd, int qt)
 	untokenize(s);
 	if ((stream = open(unmeta(s), O_RDONLY | O_NOCTTY)) == -1) {
 	    zwarn("%e: %s", errno, s);
+	    lastval = cmdoutval = 1;
 	    return newlinklist();
 	}
 	return readoutput(stream, qt);

Re: $(<nofile) doesn't set $? to non-zero

[Stephane Chazelas]

2018-03-14 10:32:54 +0000, Stephane Chazelas:
[...]
> Note that all shells are silent and return 0 for var=$(</etc)
> var=$(</dev/mem) or any file that can be opened but not read
> successfully, which IMO is not ideal.
[...]

In zsh, one can use $ERRNO though:

$ zsh -c 'zmodload zsh/system; ERRNO=0; var=$(</); echo $? $ERRNO; syserror'
0 21
Is a directory
$ zsh -c 'zmodload zsh/system; ERRNO=0; var=$(<file); echo $? $ERRNO; syserror'
zsh:1: no such file or directory: file
0 2
No such file or directory

(same for $mapfile[file] which is silent on errors).

-- 
Stephane

Re: $(<nofile) doesn't set $? to non-zero

[Stephane Chazelas]

2018-03-14 10:54:42 +0000, Peter Stephenson:
[...]
> We set lastval = cmdoutval if it's an ordinary command substitution, so it's
> easy to make this consistent with that behaviour.  Doing so seems
> unproblematic.
> 
> pws
> 
> diff --git a/Src/exec.c b/Src/exec.c
> index e5c6455..299b816 100644
> --- a/Src/exec.c
> +++ b/Src/exec.c
> @@ -4514,6 +4514,7 @@ getoutput(char *cmd, int qt)
>  	untokenize(s);
>  	if ((stream = open(unmeta(s), O_RDONLY | O_NOCTTY)) == -1) {
>  	    zwarn("%e: %s", errno, s);
> +	    lastval = cmdoutval = 1;
>  	    return newlinklist();
>  	}
>  	return readoutput(stream, qt);


Thanks. I think that code explains why we don't report an error
upon read errors ($(</), $(</dev/mem)) as we use the same
function that reads the output of normal command substitutions
where read errors are not expected (pipe).

I suppose it's the same in other shells.

Would it be worth doing some:

  	ret = readoutput(stream, qt);
	if (errno) {
	  zwarn("%e: %s", errno, s);
	  lastval = cmdoutval = 1;
	}
	return ret;

there (or something cleaner to avoid relying on errno)?

-- 
Stephane

Re: $(<nofile) doesn't set $? to non-zero

[Peter Stephenson]

On Wed, 14 Mar 2018 14:42:48 +0000
Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> Would it be worth doing some:
> 
>   	ret = readoutput(stream, qt);
> 	if (errno) {
> 	  zwarn("%e: %s", errno, s);
> 	  lastval = cmdoutval = 1;
> 	}
> 	return ret;
> 
> there (or something cleaner to avoid relying on errno)?

The return value is the linked list as we're in the context of
substitution, not command execution, which isn't easy to change without
a complete rewrite.

pws

Re: $(<nofile) doesn't set $? to non-zero

[Stephane Chazelas]

2018-03-14 14:50:04 +0000, Peter Stephenson:
> On Wed, 14 Mar 2018 14:42:48 +0000
> Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> > Would it be worth doing some:
> > 
> >   	ret = readoutput(stream, qt);
> > 	if (errno) {
> > 	  zwarn("%e: %s", errno, s);
> > 	  lastval = cmdoutval = 1;
> > 	}
> > 	return ret;
> > 
> > there (or something cleaner to avoid relying on errno)?
> 
> The return value is the linked list as we're in the context of
> substitution, not command execution, which isn't easy to change without
> a complete rewrite.
[...]

Why not just pass the error as a return argument like in the
patch below?

Then:

$ ./Src/zsh -c 'ERRNO=0; v=$(</x); echo $? $ERRNO $#v'
zsh:1: no such file or directory: /x
1 2 0
$ ./Src/zsh -c 'ERRNO=0; v=$(</); echo $? $ERRNO $#v'
zsh:1: error when reading /: is a directory
1 21 0
$ ./Src/zsh -c 'ERRNO=0; v=$(</dev/mem); echo $? $ERRNO $#v'
zsh:1: permission denied: /dev/mem
1 13 0
$ sudo ./Src/zsh -c 'ERRNO=0; v=$(</dev/mem); echo $? $ERRNO $#v'
zsh:1: error when reading /dev/mem: operation not permitted
1 1 1046296

We still expand to what could be read but signal the error like
when doing the unoptimised $(cat file).


diff --git a/Src/Modules/mapfile.c b/Src/Modules/mapfile.c
index 2503b36..7c5a1e3 100644
--- a/Src/Modules/mapfile.c
+++ b/Src/Modules/mapfile.c
@@ -197,8 +197,9 @@ get_contents(char *fname)
     val = NULL;
     if ((fd = open(fname, O_RDONLY | O_NOCTTY)) >= 0) {
 	LinkList ll;
+	int readerror;
 
-	if ((ll = readoutput(fd, 1)))
+	if ((ll = readoutput(fd, 1, 0)))
 	    val = peekfirst(ll);
     }
 #endif /* USE_MMAP */
diff --git a/Src/exec.c b/Src/exec.c
index e5c6455..32be683 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4507,6 +4507,8 @@ getoutput(char *cmd, int qt)
     if ((s = simple_redir_name(prog, REDIR_READ))) {
 	/* $(< word) */
 	int stream;
+	LinkList retval;
+	int readerror;
 
 	singsub(&s);
 	if (errflag)
@@ -4514,9 +4516,15 @@ getoutput(char *cmd, int qt)
 	untokenize(s);
 	if ((stream = open(unmeta(s), O_RDONLY | O_NOCTTY)) == -1) {
 	    zwarn("%e: %s", errno, s);
+	    lastval = cmdoutval = 1;
 	    return newlinklist();
 	}
-	return readoutput(stream, qt);
+	retval = readoutput(stream, qt, &readerror);
+	if (readerror) {
+	  zwarn("error when reading %s: %e", s, readerror);
+	  lastval = cmdoutval = 1;
+	}
+	return retval;
     }
     if (mpipe(pipes) < 0) {
 	errflag |= ERRFLAG_ERROR;
@@ -4537,7 +4545,7 @@ getoutput(char *cmd, int qt)
 	LinkList retval;
 
 	zclose(pipes[1]);
-	retval = readoutput(pipes[0], qt);
+	retval = readoutput(pipes[0], qt, 0);
 	fdtable[pipes[0]] = FDT_UNUSED;
 	waitforpid(pid, 0);		/* unblocks */
 	lastval = cmdoutval;
@@ -4562,7 +4570,7 @@ getoutput(char *cmd, int qt)
 
 /**/
 mod_export LinkList
-readoutput(int in, int qt)
+readoutput(int in, int qt, int *readerror)
 {
     LinkList ret;
     char *buf, *ptr;
@@ -4591,6 +4599,8 @@ readoutput(int in, int qt)
 	}
 	*ptr++ = c;
     }
+    if (readerror && ferror(fin))
+	*readerror = errno;
     fclose(fin);
     while (cnt && ptr[-1] == '\n')
 	ptr--, cnt--;

EILSEQ in the C locale? (Was: $(<nofile) doesn't set $? to non-zero)

[Stephane Chazelas]

2018-03-15 07:12:04 +0000, Stephane Chazelas:
[...]
> $ sudo ./Src/zsh -c 'ERRNO=0; v=$(</dev/mem); echo $? $ERRNO $#v'
> zsh:1: error when reading /dev/mem: operation not permitted
> 1 1 1046296
[...]

BTW, when swapping $#v and $ERRNO above, I noticed that I was
seeing some EILSEQ errors even in the C locale because of
"malformed characters".

I wouldn't expect to get those in a C locale or other locales
with single-byte charsets.

$ a=$'\x80' LC_ALL=C zsh -c 'ERRNO=0; echo $#a $ERRNO'
1 84
$ syserror 84
Invalid or incomplete multibyte or wide character

Maybe some optimisation could be done and things like mbstowc()
avoided when MB_CUR_MAX is 1 or something like that?

(here, I beleive my mbstowc() returns EILSEQ in the C locale for
byte values above 0x7f).

(maybe not worth the effort).

-- 
Stephane

Re: $(<nofile) doesn't set $? to non-zero

[Peter Stephenson]

On Thu, 15 Mar 2018 07:12:04 +0000
Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> 2018-03-14 14:50:04 +0000, Peter Stephenson:
> > On Wed, 14 Mar 2018 14:42:48 +0000
> > Stephane Chazelas <stephane.chazelas@gmail.com> wrote:  
> > > Would it be worth doing some:
> > > 
> > >   	ret = readoutput(stream, qt);
> > > 	if (errno) {
> > > 	  zwarn("%e: %s", errno, s);
> > > 	  lastval = cmdoutval = 1;
> > > 	}
> > > 	return ret;
> > > 
> > > there (or something cleaner to avoid relying on errno)?  
> > 
> > The return value is the linked list as we're in the context of
> > substitution, not command execution, which isn't easy to change
> > without a complete rewrite.  
> [...]
> 
> Why not just pass the error as a return argument like in the
> patch below?

That should be more consistent, thanks.  I've committed it (minor
tweaks).

pws

Re: $(<nofile) doesn't set $? to non-zero

[Stephane Chazelas]

2018-03-15 09:23:05 +0000, Peter Stephenson:
[...]
> That should be more consistent, thanks.  I've committed it (minor
> tweaks).
[...]

Sorry, my bad, I've left an unused variable in the patch.

diff --git a/Src/Modules/mapfile.c b/Src/Modules/mapfile.c
index 771e5b5fc..7a903418f 100644
--- a/Src/Modules/mapfile.c
+++ b/Src/Modules/mapfile.c
@@ -197,9 +197,8 @@ get_contents(char *fname)
     val = NULL;
     if ((fd = open(fname, O_RDONLY | O_NOCTTY)) >= 0) {
 	LinkList ll;
-	int readerror;
 
-	if ((ll = readoutput(fd, 1, &readerror)))
+	if ((ll = readoutput(fd, 1, 0)))
 	    val = peekfirst(ll);
     }
 #endif /* USE_MMAP */


I initially  thought it would be nice to report errors when the
file cannot be read in $mapfile[file], but then gave up on it on
the ground that it was not done with mmap().

Now maybe it would make sense to report errors other than ENOENT
here (even for the open)?

But then that could become annoying for things like
(($+mapfile[somefile])) (not that people should use that).

-- 
Stephane

Re: $(<nofile) doesn't set $? to non-zero

[Stephane Chazelas]

2018-03-15 11:10:14 +0000, Stephane Chazelas:
> 2018-03-15 09:23:05 +0000, Peter Stephenson:
> [...]
> > That should be more consistent, thanks.  I've committed it (minor
> > tweaks).
> [...]
> 
> Sorry, my bad, I've left an unused variable in the patch.
[...]

And worse, didn't even test the obvious $(<existing-file).

I promiss I'll do a "make test" in the future before submitting
any patch! Sorry again.


diff --git a/Src/Modules/mapfile.c b/Src/Modules/mapfile.c
index 771e5b5..7a90341 100644
--- a/Src/Modules/mapfile.c
+++ b/Src/Modules/mapfile.c
@@ -197,9 +197,8 @@ get_contents(char *fname)
     val = NULL;
     if ((fd = open(fname, O_RDONLY | O_NOCTTY)) >= 0) {
 	LinkList ll;
-	int readerror;
 
-	if ((ll = readoutput(fd, 1, &readerror)))
+	if ((ll = readoutput(fd, 1, 0)))
 	    val = peekfirst(ll);
     }
 #endif /* USE_MMAP */
diff --git a/Src/exec.c b/Src/exec.c
index ce8cf8c..35b0bb1 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4599,8 +4599,8 @@ readoutput(int in, int qt, int *readerror)
 	}
 	*ptr++ = c;
     }
-    if (readerror && ferror(fin))
-	*readerror = errno;
+    if (readerror)
+	*readerror = ferror(fin) ? errno : 0;
     fclose(fin);
     while (cnt && ptr[-1] == '\n')
 	ptr--, cnt--;

Re: EILSEQ in the C locale? (Was: $(<nofile) doesn't set $? to non-zero)

[Bart Schaefer]

On Mar 15,  7:25am, Stephane Chazelas wrote:
}
} Maybe some optimisation could be done and things like mbstowc()
} avoided when MB_CUR_MAX is 1 or something like that?

The following seems to work for this particular case ($#var) but there
are other places where we walk a wide char array with mbrtowc() so this
is not complete.

diff --git a/Src/utils.c b/Src/utils.c
index 3b589aa..ba78f29 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -5496,7 +5496,7 @@ mb_metastrlenend(char *ptr, int width, char *eptr)
     wchar_t wc;
     int num, num_in_char, complete;
 
-    if (!isset(MULTIBYTE))
+    if (!isset(MULTIBYTE) || MB_CUR_MAX == 1)
 	return eptr ? (int)(eptr - ptr) : ztrlen(ptr);
 
     laststart = ptr;