* $(<nofile) doesn't set $? to non-zero @ 2018-03-14 10:32 ` Stephane Chazelas 2018-03-14 10:54 ` Peter Stephenson 2018-03-14 11:04 ` Stephane Chazelas 0 siblings, 2 replies; 11+ messages in thread From: Stephane Chazelas @ 2018-03-14 10:32 UTC (permalink / raw) To: Zsh hackers list $ 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: $(<nofile) doesn't set $? to non-zero 2018-03-14 10:32 ` $(<nofile) doesn't set $? to non-zero Stephane Chazelas @ 2018-03-14 10:54 ` Peter Stephenson 2018-03-14 14:42 ` Stephane Chazelas 2018-03-14 11:04 ` Stephane Chazelas 1 sibling, 1 reply; 11+ messages in thread From: Peter Stephenson @ 2018-03-14 10:54 UTC (permalink / raw) To: Zsh hackers list 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); ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: $(<nofile) doesn't set $? to non-zero 2018-03-14 10:54 ` Peter Stephenson @ 2018-03-14 14:42 ` Stephane Chazelas 2018-03-14 14:50 ` Peter Stephenson 0 siblings, 1 reply; 11+ messages in thread From: Stephane Chazelas @ 2018-03-14 14:42 UTC (permalink / raw) To: Peter Stephenson; +Cc: Zsh hackers list 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: $(<nofile) doesn't set $? to non-zero 2018-03-14 14:42 ` Stephane Chazelas @ 2018-03-14 14:50 ` Peter Stephenson 2018-03-15 7:12 ` Stephane Chazelas 0 siblings, 1 reply; 11+ messages in thread From: Peter Stephenson @ 2018-03-14 14:50 UTC (permalink / raw) To: Zsh hackers list 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: $(<nofile) doesn't set $? to non-zero 2018-03-14 14:50 ` Peter Stephenson @ 2018-03-15 7:12 ` Stephane Chazelas 2018-03-15 7:25 ` EILSEQ in the C locale? (Was: $(<nofile) doesn't set $? to non-zero) Stephane Chazelas 2018-03-15 9:23 ` $(<nofile) doesn't set $? to non-zero Peter Stephenson 0 siblings, 2 replies; 11+ messages in thread From: Stephane Chazelas @ 2018-03-15 7:12 UTC (permalink / raw) To: Peter Stephenson; +Cc: Zsh hackers list 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--; ^ permalink raw reply [flat|nested] 11+ messages in thread
* EILSEQ in the C locale? (Was: $(<nofile) doesn't set $? to non-zero) 2018-03-15 7:12 ` Stephane Chazelas @ 2018-03-15 7:25 ` Stephane Chazelas 2018-03-19 0:09 ` Bart Schaefer 2018-03-15 9:23 ` $(<nofile) doesn't set $? to non-zero Peter Stephenson 1 sibling, 1 reply; 11+ messages in thread From: Stephane Chazelas @ 2018-03-15 7:25 UTC (permalink / raw) To: Peter Stephenson, Zsh hackers list 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: EILSEQ in the C locale? (Was: $(<nofile) doesn't set $? to non-zero) 2018-03-15 7:25 ` EILSEQ in the C locale? (Was: $(<nofile) doesn't set $? to non-zero) Stephane Chazelas @ 2018-03-19 0:09 ` Bart Schaefer 0 siblings, 0 replies; 11+ messages in thread From: Bart Schaefer @ 2018-03-19 0:09 UTC (permalink / raw) To: Zsh hackers list 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; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: $(<nofile) doesn't set $? to non-zero 2018-03-15 7:12 ` Stephane Chazelas 2018-03-15 7:25 ` EILSEQ in the C locale? (Was: $(<nofile) doesn't set $? to non-zero) Stephane Chazelas @ 2018-03-15 9:23 ` Peter Stephenson 2018-03-15 11:10 ` Stephane Chazelas 1 sibling, 1 reply; 11+ messages in thread From: Peter Stephenson @ 2018-03-15 9:23 UTC (permalink / raw) To: Zsh hackers list 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: $(<nofile) doesn't set $? to non-zero 2018-03-15 9:23 ` $(<nofile) doesn't set $? to non-zero Peter Stephenson @ 2018-03-15 11:10 ` Stephane Chazelas 2018-03-16 8:24 ` Stephane Chazelas 0 siblings, 1 reply; 11+ messages in thread From: Stephane Chazelas @ 2018-03-15 11:10 UTC (permalink / raw) To: Peter Stephenson; +Cc: Zsh hackers list 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: $(<nofile) doesn't set $? to non-zero 2018-03-15 11:10 ` Stephane Chazelas @ 2018-03-16 8:24 ` Stephane Chazelas 0 siblings, 0 replies; 11+ messages in thread From: Stephane Chazelas @ 2018-03-16 8:24 UTC (permalink / raw) To: Peter Stephenson, Zsh hackers list 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--; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: $(<nofile) doesn't set $? to non-zero 2018-03-14 10:32 ` $(<nofile) doesn't set $? to non-zero Stephane Chazelas 2018-03-14 10:54 ` Peter Stephenson @ 2018-03-14 11:04 ` Stephane Chazelas 1 sibling, 0 replies; 11+ messages in thread From: Stephane Chazelas @ 2018-03-14 11:04 UTC (permalink / raw) To: Zsh hackers list 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-03-19 0:09 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20180314103335epcas4p30222f0df02adda27cbddbe62075ff9ad@epcas4p3.samsung.com> 2018-03-14 10:32 ` $(<nofile) doesn't set $? to non-zero Stephane Chazelas 2018-03-14 10:54 ` Peter Stephenson 2018-03-14 14:42 ` Stephane Chazelas 2018-03-14 14:50 ` Peter Stephenson 2018-03-15 7:12 ` Stephane Chazelas 2018-03-15 7:25 ` EILSEQ in the C locale? (Was: $(<nofile) doesn't set $? to non-zero) Stephane Chazelas 2018-03-19 0:09 ` Bart Schaefer 2018-03-15 9:23 ` $(<nofile) doesn't set $? to non-zero Peter Stephenson 2018-03-15 11:10 ` Stephane Chazelas 2018-03-16 8:24 ` Stephane Chazelas 2018-03-14 11:04 ` Stephane Chazelas
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).