* Feature request: ZSH_XTRACEFD variable @ 2019-05-17 15:08 Timothée Mazzucotelli 2019-05-18 7:55 ` Stephane Chazelas 0 siblings, 1 reply; 37+ messages in thread From: Timothée Mazzucotelli @ 2019-05-17 15:08 UTC (permalink / raw) To: zsh-workers [-- Attachment #1: Type: text/plain, Size: 461 bytes --] Similar to Bash (BASH_XTRACEFD), I would like to be able to change the file descriptor to which the XTRACE output is sent. It would allow me to build a Zsh script that computes code coverage through a unit testing tool like ZUnit (inspired by Bats, for Bash). Indeed ZUnit captures stdout and stderr, therefore I cannot use the XTRACE output to compute code coverage because it's mixed up with stderr and so captured by ZUnit. Regards, Timothée Mazzucotelli ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2019-05-17 15:08 Feature request: ZSH_XTRACEFD variable Timothée Mazzucotelli @ 2019-05-18 7:55 ` Stephane Chazelas 2019-05-20 10:34 ` Stephane Chazelas 0 siblings, 1 reply; 37+ messages in thread From: Stephane Chazelas @ 2019-05-18 7:55 UTC (permalink / raw) To: Timothée Mazzucotelli; +Cc: zsh-workers 2019-05-17 15:08:57 +0000, Timothée Mazzucotelli: > Similar to Bash (BASH_XTRACEFD), I would like to be able to > change the file descriptor to which the XTRACE output is sent. [...] I second that. There are several cases where xtrace is not usable without such a feature, like when a script does var=$(myfunction 2>&1). env BASH_XTRACEFD=7 7> debug.log SHELLOPTS=xtrace bash some-cmd Is something I use often for debugging. I sometimes even go all the trouble of changing /bin/sh to a symlink to bash just for that. Being able to use zsh instead (with its much more powerful $PS4 customisation) would be very handy. The zsh equivalent of bash's SHELLOPTS can be achieved with ~/.zshenv. I'm not suggesting zsh should add a SHELLOPTS equivalent as it's quite dangerous a feature. -- Stephane ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2019-05-18 7:55 ` Stephane Chazelas @ 2019-05-20 10:34 ` Stephane Chazelas 2019-07-21 15:08 ` Timothée Mazzucotelli 0 siblings, 1 reply; 37+ messages in thread From: Stephane Chazelas @ 2019-05-20 10:34 UTC (permalink / raw) To: Timothée Mazzucotelli, zsh-workers 2019-05-18 08:55:14 +0100, Stephane Chazelas: > 2019-05-17 15:08:57 +0000, Timothée Mazzucotelli: > > Similar to Bash (BASH_XTRACEFD), I would like to be able to > > change the file descriptor to which the XTRACE output is sent. > [...] > > I second that. [...] See also: https://unix.stackexchange.com/questions/516918/direct-xtrace-output-elsewhere-than-stderr-in-zsh https://unix.stackexchange.com/questions/813/how-to-determine-where-an-environment-variable-came-from/154971#154971 https://unix.stackexchange.com/questions/367322/which-startup-file-is-being-used-by-my-shell/367353#367353 -- Stephane ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2019-05-20 10:34 ` Stephane Chazelas @ 2019-07-21 15:08 ` Timothée Mazzucotelli 2019-07-21 15:22 ` Peter Stephenson 0 siblings, 1 reply; 37+ messages in thread From: Timothée Mazzucotelli @ 2019-07-21 15:08 UTC (permalink / raw) To: zsh-workers [-- Attachment #1: Type: text/plain, Size: 149 bytes --] I'm willing to help implementing this feature (equivalent of Bash's BASH_XTRACEFD). Any advice where I should start? Who I should get in touch with? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2019-07-21 15:08 ` Timothée Mazzucotelli @ 2019-07-21 15:22 ` Peter Stephenson 2019-07-31 19:40 ` Timothée Mazzucotelli 0 siblings, 1 reply; 37+ messages in thread From: Peter Stephenson @ 2019-07-21 15:22 UTC (permalink / raw) To: zsh-workers On Sun, 2019-07-21 at 17:08 +0200, Timothée Mazzucotelli wrote: > I'm willing to help implementing this feature (equivalent of Bash's > BASH_XTRACEFD). Any advice where I should start? Who I should get in touch > with? This is the right place. Certainly feel free to investigate. Have a look at the way the FILE *xtrerr is handed in the source code. This is going to need some more careful management. Doing this without creating leaks or crashes on bad file descriptors could be interesting. In most places, once it is reopened to a different file the redirection should "just work", but there maybe some interesting special cases as we do do slightly funny things with xtrerr at some points. I think you're going to need a special integer variable that manages this file based on someone setting the FD. params.c has this sort of handling --- see various special integer variables with their own get / set functions, e.g. intsecondssetfn shows you what happens when someone sets SECONDS. You're going to have a function of that sort that instead closes and reopens xtrerr. I guess ZSH_XTRACEFD is the obvious name. Cheers pws ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2019-07-21 15:22 ` Peter Stephenson @ 2019-07-31 19:40 ` Timothée Mazzucotelli 2019-07-31 19:45 ` Timothée Mazzucotelli ` (2 more replies) 0 siblings, 3 replies; 37+ messages in thread From: Timothée Mazzucotelli @ 2019-07-31 19:40 UTC (permalink / raw) To: Peter Stephenson; +Cc: zsh-workers [-- Attachment #1: Type: text/plain, Size: 5932 bytes --] Thank you Peter for your hints. Here is a first draft: diff --git a/Src/exec.c b/Src/exec.c index 2acb2c0bc..5e550cb24 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -5393,7 +5393,7 @@ execshfunc(Shfunc shf, LinkList args) cmdsp = 0; if ((osfc = sfcontext) == SFC_NONE) sfcontext = SFC_DIRECT; - xtrerr = stderr; + xtrerr = fdopen(zsh_xtracefd, "w"); doshfunc(shf, args, 0); diff --git a/Src/init.c b/Src/init.c index 445cd3937..396f13556 100644 --- a/Src/init.c +++ b/Src/init.c @@ -609,8 +609,10 @@ init_io(char *cmd) SHTTY = -1; } - /* Send xtrace output to stderr -- see execcmd() */ - xtrerr = stderr; + /* Send xtrace output to zsh_xtracefd file descriptor -- see execcmd() */ + if (zsh_xtracefd == 0) + zsh_xtracefd = 2; + xtrerr = fdopen(zsh_xtracefd, "w"); /* Make sure the tty is opened read/write. */ if (isatty(0)) { diff --git a/Src/params.c b/Src/params.c index 1499e3a40..5e3f5cdbf 100644 --- a/Src/params.c +++ b/Src/params.c @@ -102,7 +102,8 @@ zlong lastval, /* $? */ zterm_lines, /* $LINES */ rprompt_indent, /* $ZLE_RPROMPT_INDENT */ ppid, /* $PPID */ - zsh_subshell; /* $ZSH_SUBSHELL */ + zsh_subshell, /* $ZSH_SUBSHELL */ + zsh_xtracefd; /* $ZSH_XTRACEFD */ /* $FUNCNEST */ /**/ @@ -264,6 +265,9 @@ static const struct gsu_array pipestatus_gsu = static const struct gsu_integer rprompt_indent_gsu = { intvargetfn, zlevarsetfn, rprompt_indent_unsetfn }; +static const struct gsu_integer xtracefd_gsu = +{ intvargetfn, xtracefdsetfn, xtracefdunsetfn }; + /* Nodes for special parameters for parameter hash table */ #ifdef HAVE_UNION_INIT @@ -353,6 +357,7 @@ IPDEF5("LINES", &zterm_lines, zlevar_gsu), IPDEF5U("ZLE_RPROMPT_INDENT", &rprompt_indent, rprompt_indent_gsu), IPDEF5("SHLVL", &shlvl, varinteger_gsu), IPDEF5("FUNCNEST", &zsh_funcnest, varinteger_gsu), +IPDEF5("ZSH_XTRACEFD", &zsh_xtracefd, xtracefd_gsu), /* Don't import internal integer status variables. */ #define IPDEF6(A,B,F) {{NULL,A,PM_INTEGER|PM_SPECIAL|PM_DONTIMPORT},BR((void *)B),GSU(F),10,0,NULL,NULL,NULL,0} @@ -4387,6 +4392,45 @@ setsecondstype(Param pm, int on, int off) return 0; } +/* Function to set value of special parameter `ZSH_XTRACEFD' */ + +/**/ +void +xtracefdsetfn(Param pm, zlong fd) +{ + int current_fd; + + /* Check that the given file descriptor is valid */ + if (fcntl(fd, F_GETFD) != -1 || errno != EBADF) { + current_fd = intvargetfn(pm); + /* We never close file descriptors 0 (stdin), 1 (stdout) or 2 (stderr) */ + if (current_fd > 2) { + close(current_fd); + fclose(xtrerr); + } + intvarsetfn(pm, fd); + xtrerr = fdopen(fd, "w"); + } else + zwarn("file descriptor %d is not valid", fd); + +} + +/* Function to unset value of special parameter `ZSH_XTRACEFD' */ + +/**/ +void +xtracefdunsetfn(Param pm, UNUSED(int exp)) +{ + int current_fd = intvargetfn(pm); + if (current_fd == 2) /* Nothing to do, already using stderr */ + return; + else { /* Reset to file descriptor 2 (stderr) */ + intvarsetfn(pm, 2); + if (current_fd > 1) fclose(xtrerr); /* Never close stdin or stdout */ + xtrerr = stderr; + } +} + /* Function to get value for special parameter `USERNAME' */ /**/ diff --git a/Src/utils.c b/Src/utils.c index 46cf7bcf6..93ea8044f 100644 --- a/Src/utils.c +++ b/Src/utils.c @@ -1765,7 +1765,7 @@ void printprompt4(void) { if (!xtrerr) - xtrerr = stderr; + xtrerr = fdopen(zsh_xtracefd, "w"); if (prompt4) { int l, t = opts[XTRACE]; char *s = dupstring(prompt4); And some notes: There is one place where I didn't change the code, because I wasn't sure it was needed: > Doing this without creating leaks or crashes on bad file descriptors could be interesting. About leaks, I think file pointers are all freed in the params.c set and unset functions. However I'm not sure about exec.c, line 5396, in function execshfunc, where xtrerr was previously always reassigned to stderr. Now it is reassigned to a new file pointer obtained with fdopen(zsh_xtracefd, "w") each time, and maybe this could cause memory leaks. About crashes on bad file descriptors, I used if (fcntl(fd, F_GETFD) != -1 || errno != EBADF) as a condition to test the validity of a file descriptor, but I'm not sure it can handle all the cases, or if it even makes sense. It's working with basic cases though. On Sun, Jul 21, 2019 at 5:23 PM Peter Stephenson <p.stephenson@samsung.com> wrote: > On Sun, 2019-07-21 at 17:08 +0200, Timothée Mazzucotelli wrote: > > I'm willing to help implementing this feature (equivalent of Bash's > > BASH_XTRACEFD). Any advice where I should start? Who I should get in > touch > > with? > > This is the right place. Certainly feel free to investigate. > > Have a look at the way the FILE *xtrerr is handed in the source code. This > is going to need some more careful management. Doing this without creating > leaks or crashes on bad file descriptors could be interesting. > In most places, once it is reopened to a different file the redirection > should "just work", but there maybe some interesting special cases > as we do do slightly funny things with xtrerr at some points. > > I think you're going to need a special integer variable that manages > this file based on someone setting the FD. params.c has this sort > of handling --- see various special integer variables with > their own get / set functions, e.g. intsecondssetfn shows you > what happens when someone sets SECONDS. You're going to have > a function of that sort that instead closes and reopens > xtrerr. > > I guess ZSH_XTRACEFD is the obvious name. > > Cheers > pws > > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2019-07-31 19:40 ` Timothée Mazzucotelli @ 2019-07-31 19:45 ` Timothée Mazzucotelli 2019-07-31 21:12 ` Sebastian Gniazdowski 2019-08-13 9:14 ` Peter Stephenson 2019-08-13 15:38 ` Peter Stephenson 2 siblings, 1 reply; 37+ messages in thread From: Timothée Mazzucotelli @ 2019-07-31 19:45 UTC (permalink / raw) To: Peter Stephenson; +Cc: zsh-workers [-- Attachment #1: Type: text/plain, Size: 7153 bytes --] Sorry, I hit control-enter by mistake and sent the mail before finishing writing it... There is one place where I didn't change the code, because I wasn't sure it was needed, in exec.c line 3571 if (isset(XTRACE) && xtrerr == stderr && (type < WC_SUBSH || type == WC_TIMED)) { if ((newxtrerr = fdopen(movefd(dup(fileno(stderr))), "w"))) { xtrerr = newxtrerr; fdtable[fileno(xtrerr)] = FDT_XTRACE; } } and later at line 4247 if (newxtrerr) { fil = fileno(newxtrerr); fclose(newxtrerr); xtrerr = oxtrerr; zclose(fil); } It seems these two blocks are executed only when xtrerr == stderr, which is the default case, therefore I did not modify any of this, but maybe it should be updated as well. Regards, Timothée Mazzucotelli On Wed, Jul 31, 2019 at 9:40 PM Timothée Mazzucotelli < timothee.mazzucotelli@gmail.com> wrote: > Thank you Peter for your hints. > > Here is a first draft: > > diff --git a/Src/exec.c b/Src/exec.c > index 2acb2c0bc..5e550cb24 100644 > --- a/Src/exec.c > +++ b/Src/exec.c > @@ -5393,7 +5393,7 @@ execshfunc(Shfunc shf, LinkList args) > cmdsp = 0; > if ((osfc = sfcontext) == SFC_NONE) > sfcontext = SFC_DIRECT; > - xtrerr = stderr; > + xtrerr = fdopen(zsh_xtracefd, "w"); > > doshfunc(shf, args, 0); > > diff --git a/Src/init.c b/Src/init.c > index 445cd3937..396f13556 100644 > --- a/Src/init.c > +++ b/Src/init.c > @@ -609,8 +609,10 @@ init_io(char *cmd) > SHTTY = -1; > } > > - /* Send xtrace output to stderr -- see execcmd() */ > - xtrerr = stderr; > + /* Send xtrace output to zsh_xtracefd file descriptor -- see > execcmd() */ > + if (zsh_xtracefd == 0) > + zsh_xtracefd = 2; > + xtrerr = fdopen(zsh_xtracefd, "w"); > > /* Make sure the tty is opened read/write. */ > if (isatty(0)) { > diff --git a/Src/params.c b/Src/params.c > index 1499e3a40..5e3f5cdbf 100644 > --- a/Src/params.c > +++ b/Src/params.c > @@ -102,7 +102,8 @@ zlong lastval, /* $? */ > zterm_lines, /* $LINES */ > rprompt_indent, /* $ZLE_RPROMPT_INDENT */ > ppid, /* $PPID */ > - zsh_subshell; /* $ZSH_SUBSHELL */ > + zsh_subshell, /* $ZSH_SUBSHELL */ > + zsh_xtracefd; /* $ZSH_XTRACEFD */ > > /* $FUNCNEST */ > /**/ > @@ -264,6 +265,9 @@ static const struct gsu_array pipestatus_gsu = > static const struct gsu_integer rprompt_indent_gsu = > { intvargetfn, zlevarsetfn, rprompt_indent_unsetfn }; > > +static const struct gsu_integer xtracefd_gsu = > +{ intvargetfn, xtracefdsetfn, xtracefdunsetfn }; > + > /* Nodes for special parameters for parameter hash table */ > > #ifdef HAVE_UNION_INIT > @@ -353,6 +357,7 @@ IPDEF5("LINES", &zterm_lines, zlevar_gsu), > IPDEF5U("ZLE_RPROMPT_INDENT", &rprompt_indent, rprompt_indent_gsu), > IPDEF5("SHLVL", &shlvl, varinteger_gsu), > IPDEF5("FUNCNEST", &zsh_funcnest, varinteger_gsu), > +IPDEF5("ZSH_XTRACEFD", &zsh_xtracefd, xtracefd_gsu), > > /* Don't import internal integer status variables. */ > #define IPDEF6(A,B,F) > {{NULL,A,PM_INTEGER|PM_SPECIAL|PM_DONTIMPORT},BR((void > *)B),GSU(F),10,0,NULL,NULL,NULL,0} > @@ -4387,6 +4392,45 @@ setsecondstype(Param pm, int on, int off) > return 0; > } > > +/* Function to set value of special parameter `ZSH_XTRACEFD' */ > + > +/**/ > +void > +xtracefdsetfn(Param pm, zlong fd) > +{ > + int current_fd; > + > + /* Check that the given file descriptor is valid */ > + if (fcntl(fd, F_GETFD) != -1 || errno != EBADF) { > + current_fd = intvargetfn(pm); > + /* We never close file descriptors 0 (stdin), 1 (stdout) or 2 > (stderr) */ > + if (current_fd > 2) { > + close(current_fd); > + fclose(xtrerr); > + } > + intvarsetfn(pm, fd); > + xtrerr = fdopen(fd, "w"); > + } else > + zwarn("file descriptor %d is not valid", fd); > + > +} > + > +/* Function to unset value of special parameter `ZSH_XTRACEFD' */ > + > +/**/ > +void > +xtracefdunsetfn(Param pm, UNUSED(int exp)) > +{ > + int current_fd = intvargetfn(pm); > + if (current_fd == 2) /* Nothing to do, already using stderr */ > + return; > + else { /* Reset to file descriptor 2 (stderr) */ > + intvarsetfn(pm, 2); > + if (current_fd > 1) fclose(xtrerr); /* Never close stdin or stdout > */ > + xtrerr = stderr; > + } > +} > + > /* Function to get value for special parameter `USERNAME' */ > > /**/ > diff --git a/Src/utils.c b/Src/utils.c > index 46cf7bcf6..93ea8044f 100644 > --- a/Src/utils.c > +++ b/Src/utils.c > @@ -1765,7 +1765,7 @@ void > printprompt4(void) > { > if (!xtrerr) > - xtrerr = stderr; > + xtrerr = fdopen(zsh_xtracefd, "w"); > if (prompt4) { > int l, t = opts[XTRACE]; > char *s = dupstring(prompt4); > > > And some notes: > > There is one place where I didn't change the code, because I wasn't sure > it was needed: > > > > Doing this without creating leaks or crashes on bad file descriptors > could be interesting. > > About leaks, I think file pointers are all freed in the params.c set and > unset functions. > However I'm not sure about exec.c, line 5396, in function execshfunc, > where xtrerr was > previously always reassigned to stderr. Now it is reassigned to a new file > pointer obtained > with fdopen(zsh_xtracefd, "w") each time, and maybe this could cause > memory leaks. > > About crashes on bad file descriptors, I used if (fcntl(fd, F_GETFD) != -1 > || errno != EBADF) as a condition > to test the validity of a file descriptor, but I'm not sure it can handle > all the cases, or if it even makes > sense. It's working with basic cases though. > > > > > On Sun, Jul 21, 2019 at 5:23 PM Peter Stephenson <p.stephenson@samsung.com> > wrote: > >> On Sun, 2019-07-21 at 17:08 +0200, Timothée Mazzucotelli wrote: >> > I'm willing to help implementing this feature (equivalent of Bash's >> > BASH_XTRACEFD). Any advice where I should start? Who I should get in >> touch >> > with? >> >> This is the right place. Certainly feel free to investigate. >> >> Have a look at the way the FILE *xtrerr is handed in the source code. >> This >> is going to need some more careful management. Doing this without >> creating >> leaks or crashes on bad file descriptors could be interesting. >> In most places, once it is reopened to a different file the redirection >> should "just work", but there maybe some interesting special cases >> as we do do slightly funny things with xtrerr at some points. >> >> I think you're going to need a special integer variable that manages >> this file based on someone setting the FD. params.c has this sort >> of handling --- see various special integer variables with >> their own get / set functions, e.g. intsecondssetfn shows you >> what happens when someone sets SECONDS. You're going to have >> a function of that sort that instead closes and reopens >> xtrerr. >> >> I guess ZSH_XTRACEFD is the obvious name. >> >> Cheers >> pws >> >> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2019-07-31 19:45 ` Timothée Mazzucotelli @ 2019-07-31 21:12 ` Sebastian Gniazdowski 0 siblings, 0 replies; 37+ messages in thread From: Sebastian Gniazdowski @ 2019-07-31 21:12 UTC (permalink / raw) To: Timothée Mazzucotelli; +Cc: Peter Stephenson, Zsh hackers list On Wed, 31 Jul 2019 at 21:46, Timothée Mazzucotelli <timothee.mazzucotelli@gmail.com> wrote: > > About leaks, I think file pointers are all freed in the params.c set and > > unset functions. > > However I'm not sure about exec.c, line 5396, in function execshfunc, > > where xtrerr was > > previously always reassigned to stderr. Now it is reassigned to a new file > > pointer obtained > > with fdopen(zsh_xtracefd, "w") each time, and maybe this could cause > > memory leaks. This seems to be a good use for the valgrind tool that is (?) to be added to Zsh. You can check workers/44491 and 44492. With the thing installed, you could create some test cases and run them throguh Valgrind automatically and repeatably. -- Sebastian Gniazdowski News: https://twitter.com/ZdharmaI IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin Blog: http://zdharma.org ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2019-07-31 19:40 ` Timothée Mazzucotelli 2019-07-31 19:45 ` Timothée Mazzucotelli @ 2019-08-13 9:14 ` Peter Stephenson 2019-08-13 15:38 ` Peter Stephenson 2 siblings, 0 replies; 37+ messages in thread From: Peter Stephenson @ 2019-08-13 9:14 UTC (permalink / raw) To: zsh-workers On Wed, 2019-07-31 at 21:40 +0200, Timothée Mazzucotelli wrote: > About leaks, I think file pointers are all freed in the params.c set > and unset functions. However I'm not sure about exec.c, line 5396, in > function execshfunc, where xtrerr was previously always reassigned to > stderr. Now it is reassigned to a new file pointer obtained with > fdopen(zsh_xtracefd, "w") each time, and maybe this could cause memory > leaks. Only just had a chance to look at this but yes, I think this is a problem. I'm not really sure why this assignment is needed, but if it is it should reassign to a previously opened file. Possibly the right thing to do is to keep a separate FILE *xtrace_file which always points to the file opened in xracefdsetfn or stderr if none (so no file management is ever done directly on this, it's simply a pointer to the FILE opened to the last trace file requested by the user or stderr if none). pws ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2019-07-31 19:40 ` Timothée Mazzucotelli 2019-07-31 19:45 ` Timothée Mazzucotelli 2019-08-13 9:14 ` Peter Stephenson @ 2019-08-13 15:38 ` Peter Stephenson 2019-09-10 22:19 ` Timothée Mazzucotelli 2020-04-19 10:30 ` Timothée Mazzucotelli 2 siblings, 2 replies; 37+ messages in thread From: Peter Stephenson @ 2019-08-13 15:38 UTC (permalink / raw) To: zsh-workers How about this? I've tried to make it so that if the fd is 2 or less it always uses the standard file, if it's greater it always creates or frees one, and it only creates a new file when it's requested to switch. One other fix is that we only need to fclose(xtrerr) which then closes the file descriptor. There's no way of just closing the file and not the file descriptor --- we'll need to document that the file descriptor is closed if ZSH_XTRACEFD changes again since it's a bit illogical given that the user opened the fd in the first place. pws diff --git a/Src/exec.c b/Src/exec.c index e81053d67..fe702b644 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -5394,7 +5394,7 @@ execshfunc(Shfunc shf, LinkList args) cmdsp = 0; if ((osfc = sfcontext) == SFC_NONE) sfcontext = SFC_DIRECT; - xtrerr = stderr; + xtrerr = xtrace_file; doshfunc(shf, args, 0); diff --git a/Src/init.c b/Src/init.c index 445cd3937..c51197079 100644 --- a/Src/init.c +++ b/Src/init.c @@ -609,8 +609,10 @@ init_io(char *cmd) SHTTY = -1; } - /* Send xtrace output to stderr -- see execcmd() */ - xtrerr = stderr; + /* Send xtrace output to zsh_xtracefd file descriptor -- see execcmd() */ + if (zsh_xtracefd == 0) + zsh_xtracefd = 2; + xtracefdassign(); /* Make sure the tty is opened read/write. */ if (isatty(0)) { diff --git a/Src/params.c b/Src/params.c index 1499e3a40..2347ee9c3 100644 --- a/Src/params.c +++ b/Src/params.c @@ -102,7 +102,8 @@ zlong lastval, /* $? */ zterm_lines, /* $LINES */ rprompt_indent, /* $ZLE_RPROMPT_INDENT */ ppid, /* $PPID */ - zsh_subshell; /* $ZSH_SUBSHELL */ + zsh_subshell, /* $ZSH_SUBSHELL */ + zsh_xtracefd; /* $ZSH_XTRACEFD */ /* $FUNCNEST */ /**/ @@ -264,6 +265,9 @@ static const struct gsu_array pipestatus_gsu = static const struct gsu_integer rprompt_indent_gsu = { intvargetfn, zlevarsetfn, rprompt_indent_unsetfn }; +static const struct gsu_integer xtracefd_gsu = +{ intvargetfn, xtracefdsetfn, xtracefdunsetfn }; + /* Nodes for special parameters for parameter hash table */ #ifdef HAVE_UNION_INIT @@ -353,6 +357,7 @@ IPDEF5("LINES", &zterm_lines, zlevar_gsu), IPDEF5U("ZLE_RPROMPT_INDENT", &rprompt_indent, rprompt_indent_gsu), IPDEF5("SHLVL", &shlvl, varinteger_gsu), IPDEF5("FUNCNEST", &zsh_funcnest, varinteger_gsu), +IPDEF5("ZSH_XTRACEFD", &zsh_xtracefd, xtracefd_gsu), /* Don't import internal integer status variables. */ #define IPDEF6(A,B,F) {{NULL,A,PM_INTEGER|PM_SPECIAL|PM_DONTIMPORT},BR((void *)B),GSU(F),10,0,NULL,NULL,NULL,0} @@ -4387,6 +4392,71 @@ setsecondstype(Param pm, int on, int off) return 0; } +/* Open / assign the XTRACE fd */ + +/**/ +void xtracefdassign(void) +{ + int fd = (int)zsh_xtracefd; + switch (fd) + { + case 0: /* bizarre, but handle for consistency */ + xtrerr = stdin; + break; + + case 1: + xtrerr = stdout; + break; + + case 2: + xtrerr = stderr; + break; + + default: + xtrerr = fdopen(fd, "w"); + break; + } + xtrace_file = xtrerr; +} + +/* Function to set value of special parameter `ZSH_XTRACEFD' */ + +/**/ +void +xtracefdsetfn(Param pm, zlong fd) +{ + int current_fd; + + /* Check that the given file descriptor is valid */ + if (fcntl(fd, F_GETFD) != -1 || errno != EBADF) { + current_fd = intvargetfn(pm); + /* We never close file descriptors 0 (stdin), 1 (stdout) or 2 stderr) */ + if (current_fd > 2) + fclose(xtrerr); + intvarsetfn(pm, fd); + xtracefdassign(); + } else + zwarn("file descriptor %d is not valid", fd); + +} + +/* Function to unset value of special parameter `ZSH_XTRACEFD' */ + +/**/ +void +xtracefdunsetfn(Param pm, UNUSED(int exp)) +{ + int current_fd = intvargetfn(pm); + if (current_fd == 2) /* Nothing to do, already using stderr */ + return; + else { /* Reset to file descriptor 2 (stderr) */ + intvarsetfn(pm, 2); + if (current_fd > 2) + fclose(xtrerr); /* Never close standard descriptors */ + xtrerr = xtrace_file = stderr; + } +} + /* Function to get value for special parameter `USERNAME' */ /**/ diff --git a/Src/utils.c b/Src/utils.c index 46cf7bcf6..2eee27697 100644 --- a/Src/utils.c +++ b/Src/utils.c @@ -1760,12 +1760,19 @@ checkmailpath(char **s) /**/ FILE *xtrerr = 0; +/* This records the last file XTRACE was open too. + * It's used for restoring XTRACE after a possible redirection.gggg + */ + +/**/ +FILE *xtrace_file; + /**/ void printprompt4(void) { if (!xtrerr) - xtrerr = stderr; + xtracefdassign(); if (prompt4) { int l, t = opts[XTRACE]; char *s = dupstring(prompt4); ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2019-08-13 15:38 ` Peter Stephenson @ 2019-09-10 22:19 ` Timothée Mazzucotelli 2019-09-11 8:45 ` Peter Stephenson 2020-04-19 10:30 ` Timothée Mazzucotelli 1 sibling, 1 reply; 37+ messages in thread From: Timothée Mazzucotelli @ 2019-09-10 22:19 UTC (permalink / raw) To: zsh-workers [-- Attachment #1: Type: text/plain, Size: 5912 bytes --] Well it looks good! I pulled the changes for using valgrind, as previously suggested by Sebastian Gniazdowski, and ran the test suite, but got way more than just one leak: 255 actually (funny number), totaling 57,299 bytes definitely lost in 596 blocks, and 2,274 bytes indirectly lost in 557 blocks. Is this expected? Unless you think the code is not ready yet, I'm going to look at the tests now, and try to write some for this new xtracefd feature. Timothée On Tue, Aug 13, 2019 at 5:39 PM Peter Stephenson <p.stephenson@samsung.com> wrote: > How about this? > > I've tried to make it so that if the fd is 2 or less it always uses > the standard file, if it's greater it always creates or frees one, and > it only creates a new file when it's requested to switch. > > One other fix is that we only need to fclose(xtrerr) which then closes > the file descriptor. There's no way of just closing the file and not > the file descriptor --- we'll need to document that the file descriptor > is closed if ZSH_XTRACEFD changes again since it's a bit illogical given > that the user opened the fd in the first place. > > pws > > diff --git a/Src/exec.c b/Src/exec.c > index e81053d67..fe702b644 100644 > --- a/Src/exec.c > +++ b/Src/exec.c > @@ -5394,7 +5394,7 @@ execshfunc(Shfunc shf, LinkList args) > cmdsp = 0; > if ((osfc = sfcontext) == SFC_NONE) > sfcontext = SFC_DIRECT; > - xtrerr = stderr; > + xtrerr = xtrace_file; > > doshfunc(shf, args, 0); > > diff --git a/Src/init.c b/Src/init.c > index 445cd3937..c51197079 100644 > --- a/Src/init.c > +++ b/Src/init.c > @@ -609,8 +609,10 @@ init_io(char *cmd) > SHTTY = -1; > } > > - /* Send xtrace output to stderr -- see execcmd() */ > - xtrerr = stderr; > + /* Send xtrace output to zsh_xtracefd file descriptor -- see > execcmd() */ > + if (zsh_xtracefd == 0) > + zsh_xtracefd = 2; > + xtracefdassign(); > > /* Make sure the tty is opened read/write. */ > if (isatty(0)) { > diff --git a/Src/params.c b/Src/params.c > index 1499e3a40..2347ee9c3 100644 > --- a/Src/params.c > +++ b/Src/params.c > @@ -102,7 +102,8 @@ zlong lastval, /* $? */ > zterm_lines, /* $LINES */ > rprompt_indent, /* $ZLE_RPROMPT_INDENT */ > ppid, /* $PPID */ > - zsh_subshell; /* $ZSH_SUBSHELL */ > + zsh_subshell, /* $ZSH_SUBSHELL */ > + zsh_xtracefd; /* $ZSH_XTRACEFD */ > > /* $FUNCNEST */ > /**/ > @@ -264,6 +265,9 @@ static const struct gsu_array pipestatus_gsu = > static const struct gsu_integer rprompt_indent_gsu = > { intvargetfn, zlevarsetfn, rprompt_indent_unsetfn }; > > +static const struct gsu_integer xtracefd_gsu = > +{ intvargetfn, xtracefdsetfn, xtracefdunsetfn }; > + > /* Nodes for special parameters for parameter hash table */ > > #ifdef HAVE_UNION_INIT > @@ -353,6 +357,7 @@ IPDEF5("LINES", &zterm_lines, zlevar_gsu), > IPDEF5U("ZLE_RPROMPT_INDENT", &rprompt_indent, rprompt_indent_gsu), > IPDEF5("SHLVL", &shlvl, varinteger_gsu), > IPDEF5("FUNCNEST", &zsh_funcnest, varinteger_gsu), > +IPDEF5("ZSH_XTRACEFD", &zsh_xtracefd, xtracefd_gsu), > > /* Don't import internal integer status variables. */ > #define IPDEF6(A,B,F) > {{NULL,A,PM_INTEGER|PM_SPECIAL|PM_DONTIMPORT},BR((void > *)B),GSU(F),10,0,NULL,NULL,NULL,0} > @@ -4387,6 +4392,71 @@ setsecondstype(Param pm, int on, int off) > return 0; > } > > +/* Open / assign the XTRACE fd */ > + > +/**/ > +void xtracefdassign(void) > +{ > + int fd = (int)zsh_xtracefd; > + switch (fd) > + { > + case 0: /* bizarre, but handle for consistency */ > + xtrerr = stdin; > + break; > + > + case 1: > + xtrerr = stdout; > + break; > + > + case 2: > + xtrerr = stderr; > + break; > + > + default: > + xtrerr = fdopen(fd, "w"); > + break; > + } > + xtrace_file = xtrerr; > +} > + > +/* Function to set value of special parameter `ZSH_XTRACEFD' */ > + > +/**/ > +void > +xtracefdsetfn(Param pm, zlong fd) > +{ > + int current_fd; > + > + /* Check that the given file descriptor is valid */ > + if (fcntl(fd, F_GETFD) != -1 || errno != EBADF) { > + current_fd = intvargetfn(pm); > + /* We never close file descriptors 0 (stdin), 1 (stdout) or 2 > stderr) */ > + if (current_fd > 2) > + fclose(xtrerr); > + intvarsetfn(pm, fd); > + xtracefdassign(); > + } else > + zwarn("file descriptor %d is not valid", fd); > + > +} > + > +/* Function to unset value of special parameter `ZSH_XTRACEFD' */ > + > +/**/ > +void > +xtracefdunsetfn(Param pm, UNUSED(int exp)) > +{ > + int current_fd = intvargetfn(pm); > + if (current_fd == 2) /* Nothing to do, already using stderr */ > + return; > + else { /* Reset to file descriptor 2 (stderr) */ > + intvarsetfn(pm, 2); > + if (current_fd > 2) > + fclose(xtrerr); /* Never close standard descriptors */ > + xtrerr = xtrace_file = stderr; > + } > +} > + > /* Function to get value for special parameter `USERNAME' */ > > /**/ > diff --git a/Src/utils.c b/Src/utils.c > index 46cf7bcf6..2eee27697 100644 > --- a/Src/utils.c > +++ b/Src/utils.c > @@ -1760,12 +1760,19 @@ checkmailpath(char **s) > /**/ > FILE *xtrerr = 0; > > +/* This records the last file XTRACE was open too. > + * It's used for restoring XTRACE after a possible redirection.gggg > + */ > + > +/**/ > +FILE *xtrace_file; > + > /**/ > void > printprompt4(void) > { > if (!xtrerr) > - xtrerr = stderr; > + xtracefdassign(); > if (prompt4) { > int l, t = opts[XTRACE]; > char *s = dupstring(prompt4); > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2019-09-10 22:19 ` Timothée Mazzucotelli @ 2019-09-11 8:45 ` Peter Stephenson 0 siblings, 0 replies; 37+ messages in thread From: Peter Stephenson @ 2019-09-11 8:45 UTC (permalink / raw) To: zsh-workers On Wed, 2019-09-11 at 00:19 +0200, Timothée Mazzucotelli wrote: > Well it looks good! > > I pulled the changes for using valgrind, as previously suggested by > Sebastian Gniazdowski, > and ran the test suite, but got way more than just one leak: 255 actually > (funny number), > totaling 57,299 bytes definitely lost in 596 blocks, and 2,274 bytes > indirectly lost in 557 blocks. > Is this expected? Sounds like that's going to need tracking down. Valgrind run in the right way should give enough information to do this. pws ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2019-08-13 15:38 ` Peter Stephenson 2019-09-10 22:19 ` Timothée Mazzucotelli @ 2020-04-19 10:30 ` Timothée Mazzucotelli 2020-04-20 14:09 ` Peter Stephenson 2020-05-05 20:50 ` Daniel Shahaf 1 sibling, 2 replies; 37+ messages in thread From: Timothée Mazzucotelli @ 2020-04-19 10:30 UTC (permalink / raw) To: Peter Stephenson, zsh-workers [-- Attachment #1.1: Type: text/plain, Size: 5452 bytes --] Hello :) I managed to write a test for the ZSH_XTRACEFD feature in A04redirect.ztst, and am planning to add more tests, but maybe you'd have some hints on what I should test? Patch in attachment. On Tue, Aug 13, 2019 at 5:39 PM Peter Stephenson <p.stephenson@samsung.com> wrote: > How about this? > > I've tried to make it so that if the fd is 2 or less it always uses > the standard file, if it's greater it always creates or frees one, and > it only creates a new file when it's requested to switch. > > One other fix is that we only need to fclose(xtrerr) which then closes > the file descriptor. There's no way of just closing the file and not > the file descriptor --- we'll need to document that the file descriptor > is closed if ZSH_XTRACEFD changes again since it's a bit illogical given > that the user opened the fd in the first place. > > pws > > diff --git a/Src/exec.c b/Src/exec.c > index e81053d67..fe702b644 100644 > --- a/Src/exec.c > +++ b/Src/exec.c > @@ -5394,7 +5394,7 @@ execshfunc(Shfunc shf, LinkList args) > cmdsp = 0; > if ((osfc = sfcontext) == SFC_NONE) > sfcontext = SFC_DIRECT; > - xtrerr = stderr; > + xtrerr = xtrace_file; > > doshfunc(shf, args, 0); > > diff --git a/Src/init.c b/Src/init.c > index 445cd3937..c51197079 100644 > --- a/Src/init.c > +++ b/Src/init.c > @@ -609,8 +609,10 @@ init_io(char *cmd) > SHTTY = -1; > } > > - /* Send xtrace output to stderr -- see execcmd() */ > - xtrerr = stderr; > + /* Send xtrace output to zsh_xtracefd file descriptor -- see > execcmd() */ > + if (zsh_xtracefd == 0) > + zsh_xtracefd = 2; > + xtracefdassign(); > > /* Make sure the tty is opened read/write. */ > if (isatty(0)) { > diff --git a/Src/params.c b/Src/params.c > index 1499e3a40..2347ee9c3 100644 > --- a/Src/params.c > +++ b/Src/params.c > @@ -102,7 +102,8 @@ zlong lastval, /* $? */ > zterm_lines, /* $LINES */ > rprompt_indent, /* $ZLE_RPROMPT_INDENT */ > ppid, /* $PPID */ > - zsh_subshell; /* $ZSH_SUBSHELL */ > + zsh_subshell, /* $ZSH_SUBSHELL */ > + zsh_xtracefd; /* $ZSH_XTRACEFD */ > > /* $FUNCNEST */ > /**/ > @@ -264,6 +265,9 @@ static const struct gsu_array pipestatus_gsu = > static const struct gsu_integer rprompt_indent_gsu = > { intvargetfn, zlevarsetfn, rprompt_indent_unsetfn }; > > +static const struct gsu_integer xtracefd_gsu = > +{ intvargetfn, xtracefdsetfn, xtracefdunsetfn }; > + > /* Nodes for special parameters for parameter hash table */ > > #ifdef HAVE_UNION_INIT > @@ -353,6 +357,7 @@ IPDEF5("LINES", &zterm_lines, zlevar_gsu), > IPDEF5U("ZLE_RPROMPT_INDENT", &rprompt_indent, rprompt_indent_gsu), > IPDEF5("SHLVL", &shlvl, varinteger_gsu), > IPDEF5("FUNCNEST", &zsh_funcnest, varinteger_gsu), > +IPDEF5("ZSH_XTRACEFD", &zsh_xtracefd, xtracefd_gsu), > > /* Don't import internal integer status variables. */ > #define IPDEF6(A,B,F) > {{NULL,A,PM_INTEGER|PM_SPECIAL|PM_DONTIMPORT},BR((void > *)B),GSU(F),10,0,NULL,NULL,NULL,0} > @@ -4387,6 +4392,71 @@ setsecondstype(Param pm, int on, int off) > return 0; > } > > +/* Open / assign the XTRACE fd */ > + > +/**/ > +void xtracefdassign(void) > +{ > + int fd = (int)zsh_xtracefd; > + switch (fd) > + { > + case 0: /* bizarre, but handle for consistency */ > + xtrerr = stdin; > + break; > + > + case 1: > + xtrerr = stdout; > + break; > + > + case 2: > + xtrerr = stderr; > + break; > + > + default: > + xtrerr = fdopen(fd, "w"); > + break; > + } > + xtrace_file = xtrerr; > +} > + > +/* Function to set value of special parameter `ZSH_XTRACEFD' */ > + > +/**/ > +void > +xtracefdsetfn(Param pm, zlong fd) > +{ > + int current_fd; > + > + /* Check that the given file descriptor is valid */ > + if (fcntl(fd, F_GETFD) != -1 || errno != EBADF) { > + current_fd = intvargetfn(pm); > + /* We never close file descriptors 0 (stdin), 1 (stdout) or 2 > stderr) */ > + if (current_fd > 2) > + fclose(xtrerr); > + intvarsetfn(pm, fd); > + xtracefdassign(); > + } else > + zwarn("file descriptor %d is not valid", fd); > + > +} > + > +/* Function to unset value of special parameter `ZSH_XTRACEFD' */ > + > +/**/ > +void > +xtracefdunsetfn(Param pm, UNUSED(int exp)) > +{ > + int current_fd = intvargetfn(pm); > + if (current_fd == 2) /* Nothing to do, already using stderr */ > + return; > + else { /* Reset to file descriptor 2 (stderr) */ > + intvarsetfn(pm, 2); > + if (current_fd > 2) > + fclose(xtrerr); /* Never close standard descriptors */ > + xtrerr = xtrace_file = stderr; > + } > +} > + > /* Function to get value for special parameter `USERNAME' */ > > /**/ > diff --git a/Src/utils.c b/Src/utils.c > index 46cf7bcf6..2eee27697 100644 > --- a/Src/utils.c > +++ b/Src/utils.c > @@ -1760,12 +1760,19 @@ checkmailpath(char **s) > /**/ > FILE *xtrerr = 0; > > +/* This records the last file XTRACE was open too. > + * It's used for restoring XTRACE after a possible redirection.gggg > + */ > + > +/**/ > +FILE *xtrace_file; > + > /**/ > void > printprompt4(void) > { > if (!xtrerr) > - xtrerr = stderr; > + xtracefdassign(); > if (prompt4) { > int l, t = opts[XTRACE]; > char *s = dupstring(prompt4); > [-- Attachment #1.2: Type: text/html, Size: 7005 bytes --] [-- Attachment #2: 0001-44752-Implement-ZSH_XTRACEFD-feature.patch --] [-- Type: text/x-patch, Size: 5280 bytes --] From 70a069a43fc15eff5163290ecaa79d7c71708460 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Mazzucotelli?= <pawamoy@pm.me> Date: Sat, 18 Apr 2020 17:22:03 +0200 Subject: [PATCH] 44752: Implement ZSH_XTRACEFD feature --- Src/exec.c | 2 +- Src/init.c | 6 ++-- Src/params.c | 72 ++++++++++++++++++++++++++++++++++++++++++- Src/utils.c | 9 +++++- Test/A04redirect.ztst | 10 ++++++ 5 files changed, 94 insertions(+), 5 deletions(-) diff --git a/Src/exec.c b/Src/exec.c index 2b8e2167f..0e34f5f96 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -5421,7 +5421,7 @@ execshfunc(Shfunc shf, LinkList args) cmdsp = 0; if ((osfc = sfcontext) == SFC_NONE) sfcontext = SFC_DIRECT; - xtrerr = stderr; + xtrerr = xtrace_file; doshfunc(shf, args, 0); diff --git a/Src/init.c b/Src/init.c index 3d6c94d04..89c50b17e 100644 --- a/Src/init.c +++ b/Src/init.c @@ -616,8 +616,10 @@ init_io(char *cmd) SHTTY = -1; } - /* Send xtrace output to stderr -- see execcmd() */ - xtrerr = stderr; + /* Send xtrace output to zsh_xtracefd file descriptor -- see execcmd() */ + if (zsh_xtracefd == 0) + zsh_xtracefd = 2; + xtracefdassign(); /* Make sure the tty is opened read/write. */ if (isatty(0)) { diff --git a/Src/params.c b/Src/params.c index 863b32600..569c21304 100644 --- a/Src/params.c +++ b/Src/params.c @@ -106,7 +106,8 @@ zlong lastval, /* $? */ zterm_lines, /* $LINES */ rprompt_indent, /* $ZLE_RPROMPT_INDENT */ ppid, /* $PPID */ - zsh_subshell; /* $ZSH_SUBSHELL */ + zsh_subshell, /* $ZSH_SUBSHELL */ + zsh_xtracefd; /* $ZSH_XTRACEFD */ /* $FUNCNEST */ /**/ @@ -268,6 +269,9 @@ static const struct gsu_array pipestatus_gsu = static const struct gsu_integer rprompt_indent_gsu = { intvargetfn, zlevarsetfn, rprompt_indent_unsetfn }; +static const struct gsu_integer xtracefd_gsu = +{ intvargetfn, xtracefdsetfn, xtracefdunsetfn }; + /* Nodes for special parameters for parameter hash table */ #ifdef HAVE_UNION_INIT @@ -357,6 +361,7 @@ IPDEF5("LINES", &zterm_lines, zlevar_gsu), IPDEF5U("ZLE_RPROMPT_INDENT", &rprompt_indent, rprompt_indent_gsu), IPDEF5("SHLVL", &shlvl, varinteger_gsu), IPDEF5("FUNCNEST", &zsh_funcnest, varinteger_gsu), +IPDEF5("ZSH_XTRACEFD", &zsh_xtracefd, xtracefd_gsu), /* Don't import internal integer status variables. */ #define IPDEF6(A,B,F) {{NULL,A,PM_INTEGER|PM_SPECIAL|PM_DONTIMPORT},BR((void *)B),GSU(F),10,0,NULL,NULL,NULL,0} @@ -4399,6 +4404,71 @@ setsecondstype(Param pm, int on, int off) return 0; } +/* Open / assign the XTRACE fd */ + +/**/ +void xtracefdassign(void) +{ + int fd = (int)zsh_xtracefd; + switch (fd) + { + case 0: /* bizarre, but handle for consistency */ + xtrerr = stdin; + break; + + case 1: + xtrerr = stdout; + break; + + case 2: + xtrerr = stderr; + break; + + default: + xtrerr = fdopen(fd, "w"); + break; + } + xtrace_file = xtrerr; +} + +/* Function to set value of special parameter `ZSH_XTRACEFD' */ + +/**/ +void +xtracefdsetfn(Param pm, zlong fd) +{ + int current_fd; + + /* Check that the given file descriptor is valid */ + if (fcntl(fd, F_GETFD) != -1 || errno != EBADF) { + current_fd = intvargetfn(pm); + /* We never close file descriptors 0 (stdin), 1 (stdout) or 2 stderr) */ + if (current_fd > 2) + fclose(xtrerr); + intvarsetfn(pm, fd); + xtracefdassign(); + } else + zwarn("file descriptor %d is not valid", fd); + +} + +/* Function to unset value of special parameter `ZSH_XTRACEFD' */ + +/**/ +void +xtracefdunsetfn(Param pm, UNUSED(int exp)) +{ + int current_fd = intvargetfn(pm); + if (current_fd == 2) /* Nothing to do, already using stderr */ + return; + else { /* Reset to file descriptor 2 (stderr) */ + intvarsetfn(pm, 2); + if (current_fd > 2) + fclose(xtrerr); /* Never close standard descriptors */ + xtrerr = xtrace_file = stderr; + } +} + /* Function to get value for special parameter `USERNAME' */ /**/ diff --git a/Src/utils.c b/Src/utils.c index 69885fed3..a817c9aa8 100644 --- a/Src/utils.c +++ b/Src/utils.c @@ -1760,12 +1760,19 @@ checkmailpath(char **s) /**/ FILE *xtrerr = 0; +/* This records the last file XTRACE was open too. + * It's used for restoring XTRACE after a possible redirection. + */ + +/**/ +FILE *xtrace_file; + /**/ void printprompt4(void) { if (!xtrerr) - xtrerr = stderr; + xtracefdassign(); if (prompt4) { int l, t = opts[XTRACE]; char *s = dupstring(prompt4); diff --git a/Test/A04redirect.ztst b/Test/A04redirect.ztst index d60519064..fd06854e1 100644 --- a/Test/A04redirect.ztst +++ b/Test/A04redirect.ztst @@ -708,3 +708,13 @@ cat <&$testfd 0:Regression test for here document with fd declarator > This is, in some sense, a here document. + + rm -f redir + set -x + ZSH_XTRACEFD=4 print 'This is ZSH_XTRACEFD redir' 4>redir + set +x + cat redir +0:Redirect xtrace output to ZSH_XTRACEFD file descriptor +>This is ZSH_XTRACEFD redir +>+(eval):3> print 'This is ZSH_XTRACEFD redir' +?+(eval):3> ZSH_XTRACEFD=4 +(eval):4> set +x -- 2.26.0 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-04-19 10:30 ` Timothée Mazzucotelli @ 2020-04-20 14:09 ` Peter Stephenson 2020-05-02 18:02 ` Timothée Mazzucotelli 2020-05-05 20:50 ` Daniel Shahaf 1 sibling, 1 reply; 37+ messages in thread From: Peter Stephenson @ 2020-04-20 14:09 UTC (permalink / raw) To: Timothée Mazzucotelli, zsh-workers > On 19 April 2020 at 11:30 Timothée Mazzucotelli <timothee.mazzucotelli@gmail.com> wrote: > I managed to write a test for the ZSH_XTRACEFD feature in A04redirect.ztst, > and am planning to add more tests, but maybe you'd have some hints on what > I should test? I think the interesting things are where the variable is changing dynamically, e.g. has a local value in function and then changes back. We should check doing that repeatedly is robust. If that works that's a pretty good first pass test at robustness. pws ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-04-20 14:09 ` Peter Stephenson @ 2020-05-02 18:02 ` Timothée Mazzucotelli 2020-05-02 21:15 ` Bart Schaefer 2020-05-03 17:30 ` Peter Stephenson 0 siblings, 2 replies; 37+ messages in thread From: Timothée Mazzucotelli @ 2020-05-02 18:02 UTC (permalink / raw) To: Peter Stephenson; +Cc: zsh-workers [-- Attachment #1: Type: text/plain, Size: 2071 bytes --] I wrote such a test and noticed that file descriptors were being closed each time ZSH_XTRACEFD was (re)assigned, even as a local variable. So I removed the code lines closing the previous file descriptor in xtracefdsetfn, and it seemed to work well. However it seems that leaving the scope of a function where ZSH_XTRACEFD was assigned as a local variable does not unset it, and therefore leaves the file descriptor opened. I thought leaving the scope of a function would implicitly "unset" all variables local to this scope, but it seems it's not the case. I'm not sure how to fix that. How to make sure the file descriptor of a local ZSH_XTRACEFD is closed when leaving the function scope? Or, how to make sure a local ZSH_XTRACEFD is unset when leaving the function scope? Or, how to call xtracefdunsetfn when leaving the function scope? I pasted below the test that should pass if the local file descriptor was properly closed. rm -f redir A() { local ZSH_XTRACEFD=5 B print 'Function A to file descriptor 5' unset ZSH_XTRACEFD print 'Function A to file descriptor 2' } B() { local ZSH_XTRACEFD=6 print 'Function B to file descriptor 6' } exec 4>redir4 5>redir5 6>redir6 ZSH_XTRACEFD=4 set -x print 'Main to file descriptor 4' A ZSH_XTRACEFD=5 ZSH_XTRACEFD=6 unset ZSH_XTRACEFD set +x cat redir4 print cat redir5 print cat redir6 0:Scoped ZSH_XTRACEFD correctly set and restored >Main to file descriptor 4 >Function B to file descriptor 6 >Function A to file descriptor 5 >Function A to file descriptor 2 >+(eval):16> print 'Main to file descriptor 4' >+(eval):17> A >+A:1> local ZSH_XTRACEFD=5 >+(eval):18> ZSH_XTRACEFD=5 >+(eval):19> ZSH_XTRACEFD=6 >+(eval):20> unset ZSH_XTRACEFD > >+A:2> B >+B:1> local ZSH_XTRACEFD=6 >+A:3> print 'Function A to file descriptor 5' >+A:4> unset ZSH_XTRACEFD > >+B:2> print 'Function B to file descriptor 6' ?+A:5> print 'Function A to file descriptor 2' ?(eval):18: file descriptor 5 is not valid ?(eval):19: file descriptor 6 is not valid ?+(eval):21> set +x ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-02 18:02 ` Timothée Mazzucotelli @ 2020-05-02 21:15 ` Bart Schaefer 2020-05-03 0:06 ` Daniel Shahaf 2020-05-03 17:30 ` Peter Stephenson 1 sibling, 1 reply; 37+ messages in thread From: Bart Schaefer @ 2020-05-02 21:15 UTC (permalink / raw) To: Timothée Mazzucotelli; +Cc: Peter Stephenson, zsh-workers On Sat, May 2, 2020 at 11:03 AM Timothée Mazzucotelli <timothee.mazzucotelli@gmail.com> wrote: > > How to make sure the file descriptor of a local ZSH_XTRACEFD is closed when > leaving the function scope? > Or, how to make sure a local ZSH_XTRACEFD is unset when leaving the > function scope? I think you're going to have to declare ZSH_XTRACEFD to be a "special" variable, with all the extra plumbing that entails. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-02 21:15 ` Bart Schaefer @ 2020-05-03 0:06 ` Daniel Shahaf 2020-05-03 4:43 ` Roman Perepelitsa 2020-05-03 6:01 ` Stephane Chazelas 0 siblings, 2 replies; 37+ messages in thread From: Daniel Shahaf @ 2020-05-03 0:06 UTC (permalink / raw) To: Bart Schaefer; +Cc: Timothée Mazzucotelli, Peter Stephenson, zsh-workers Bart Schaefer wrote on Sat, 02 May 2020 14:15 -0700: > On Sat, May 2, 2020 at 11:03 AM Timothée Mazzucotelli > <timothee.mazzucotelli@gmail.com> wrote: > > > > How to make sure the file descriptor of a local ZSH_XTRACEFD is closed when > > leaving the function scope? > > Or, how to make sure a local ZSH_XTRACEFD is unset when leaving the > > function scope? > > I think you're going to have to declare ZSH_XTRACEFD to be a "special" > variable, with all the extra plumbing that entails. It's already special, via the IPDEF5() macro. The end-of-scope processing happens in scanendscope(). If the parameter weren't special, unsetparam_pm() would be called and would call the unsetfn. However, the codepath for (non-removable) specials doesn't call the unsetfn; it just calls the setfn again. I would have expected the unsetfn to be called for any special parameter. Cheers, Daniel P.S. Shouldn't ZSH_XTRACEFD be declared with PM_DONTIMPORT? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-03 0:06 ` Daniel Shahaf @ 2020-05-03 4:43 ` Roman Perepelitsa 2020-05-03 16:21 ` Daniel Shahaf 2020-05-03 19:54 ` Peter Stephenson 2020-05-03 6:01 ` Stephane Chazelas 1 sibling, 2 replies; 37+ messages in thread From: Roman Perepelitsa @ 2020-05-03 4:43 UTC (permalink / raw) To: Daniel Shahaf Cc: Bart Schaefer, Timothée Mazzucotelli, Peter Stephenson, zsh-workers On Sun, May 3, 2020 at 2:07 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > I would have expected the unsetfn to be called for any special parameter. I've bumped into a similar issue with LC_* parameters. Here are a couple of test cases: 1. ( unset -m 'LC_*|LANG' export LC_CTYPE='en_US.UTF-8' # set this to any UTF-8 locale you have echo '\u276F' # this works () { local LC_ALL=C } echo '\u276F' # this doesn't work ) 2. ( unset -m 'LC_*|LANG' LC_COLLATE=en_US.UTF-8 x=(-a --b) print -r -- ${(on)x} # this prints "-a --b" () { local LC_ALL= LC_COLLATE=C print -r -- ${(on)x} } print -r -- ${(on)x} # this prints "--b -a" ) Roman. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-03 4:43 ` Roman Perepelitsa @ 2020-05-03 16:21 ` Daniel Shahaf 2020-05-03 19:54 ` Peter Stephenson 1 sibling, 0 replies; 37+ messages in thread From: Daniel Shahaf @ 2020-05-03 16:21 UTC (permalink / raw) To: Roman Perepelitsa Cc: Bart Schaefer, Timothée Mazzucotelli, Peter Stephenson, zsh-workers Roman Perepelitsa wrote on Sun, 03 May 2020 06:43 +0200: > On Sun, May 3, 2020 at 2:07 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > > I would have expected the unsetfn to be called for any special parameter. > > I've bumped into a similar issue with LC_* parameters. Here are a > couple of test cases: > LC_ALL's unsetfn is stdunsetfn(), so even if unsetfn were called, I don't think the locale would be restored. Agree it's a bug, though. Anyone interested in writing the patch, or a regression test? Cheers, Daniel > 1. > > ( > unset -m 'LC_*|LANG' > export LC_CTYPE='en_US.UTF-8' # set this to any UTF-8 locale you have > echo '\u276F' # this works > () { > local LC_ALL=C > } > echo '\u276F' # this doesn't work > ) > > 2. > > ( > unset -m 'LC_*|LANG' > LC_COLLATE=en_US.UTF-8 > x=(-a --b) > print -r -- ${(on)x} # this prints "-a --b" > () { > local LC_ALL= LC_COLLATE=C > print -r -- ${(on)x} > } > print -r -- ${(on)x} # this prints "--b -a" > ) > > Roman. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-03 4:43 ` Roman Perepelitsa 2020-05-03 16:21 ` Daniel Shahaf @ 2020-05-03 19:54 ` Peter Stephenson 2020-05-03 21:06 ` Daniel Shahaf 1 sibling, 1 reply; 37+ messages in thread From: Peter Stephenson @ 2020-05-03 19:54 UTC (permalink / raw) To: Zsh hackers list [-- Attachment #1: Type: text/plain, Size: 957 bytes --] On Sun, 2020-05-03 at 06:43 +0200, Roman Perepelitsa wrote: > On Sun, May 3, 2020 at 2:07 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > > I would have expected the unsetfn to be called for any special parameter. > > I've bumped into a similar issue with LC_* parameters. Here are a > couple of test cases: > > 1. > > ( > unset -m 'LC_*|LANG' > export LC_CTYPE='en_US.UTF-8' # set this to any UTF-8 locale you have > echo '\u276F' # this works > () { > local LC_ALL=C > } > echo '\u276F' # this doesn't work > ) > > 2. > > ( > unset -m 'LC_*|LANG' > LC_COLLATE=en_US.UTF-8 > x=(-a --b) > print -r -- ${(on)x} # this prints "-a --b" > () { > local LC_ALL= LC_COLLATE=C > print -r -- ${(on)x} > } > print -r -- ${(on)x} # this prints "--b -a" > ) Something like the attached? Slightly but not extensively tested, so I could easily have missed something. pws [-- Attachment #2: restore_locale.dif --] [-- Type: text/x-patch, Size: 1564 bytes --] diff --git a/Src/params.c b/Src/params.c index 863b32600..122f5da7d 100644 --- a/Src/params.c +++ b/Src/params.c @@ -5569,6 +5569,14 @@ startparamscope(void) locallevel++; } +#ifdef USE_LOCALE +/* + * Flag that one of the special LC_ functions or LANG changed on scope + * end + */ +static int lc_update_needed; +#endif /* USE_LOCALE */ + /* End a parameter scope: delete the parameters local to the scope. */ /**/ @@ -5579,7 +5587,28 @@ endparamscope(void) locallevel--; /* This pops anything from a higher locallevel */ saveandpophiststack(0, HFILE_USE_OPTIONS); +#ifdef USE_LOCALE + lc_update_needed = 0; +#endif scanhashtable(paramtab, 0, 0, 0, scanendscope, 0); +#ifdef USE_LOCALE + if (lc_update_needed) + { + /* Locale changed --- ensure it is restored. */ + char *val; + if ((val = getsparam_u("LC_ALL")) && *val) { + setlocale(LC_ALL, val); + } else { + struct localename *ln; + if ((val = getsparam_u("LANG")) && *val) + setlang(val); + for (ln = lc_names; ln->name; ln++) { + if ((val = getsparam_u(ln->name)) && *val) + setlocale(ln->category, val); + } + } + } +#endif /* USE_LOCALE */ unqueue_signals(); } @@ -5600,6 +5629,11 @@ scanendscope(HashNode hn, UNUSED(int flags)) */ Param tpm = pm->old; +#ifdef USE_LOCALE + if (!strncmp(pm->node.nam, "LC_", 3) || + !strcmp(pm->node.nam, "LANG")) + lc_update_needed = 1; +#endif if (!strcmp(pm->node.nam, "SECONDS")) { setsecondstype(pm, PM_TYPE(tpm->node.flags), PM_TYPE(pm->node.flags)); ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-03 19:54 ` Peter Stephenson @ 2020-05-03 21:06 ` Daniel Shahaf 2020-05-04 8:35 ` Peter Stephenson 0 siblings, 1 reply; 37+ messages in thread From: Daniel Shahaf @ 2020-05-03 21:06 UTC (permalink / raw) To: zsh-workers Peter Stephenson wrote on Sun, 03 May 2020 20:54 +0100: > On Sun, 2020-05-03 at 06:43 +0200, Roman Perepelitsa wrote: > > On Sun, May 3, 2020 at 2:07 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > > > > I would have expected the unsetfn to be called for any special parameter. > > > > I've bumped into a similar issue with LC_* parameters. Here are a > > couple of test cases: > > > > 1. > > > > ( > > unset -m 'LC_*|LANG' > > export LC_CTYPE='en_US.UTF-8' # set this to any UTF-8 locale you have > > echo '\u276F' # this works > > () { > > local LC_ALL=C > > } > > echo '\u276F' # this doesn't work > > ) > > > > 2. > > > > ( > > unset -m 'LC_*|LANG' > > LC_COLLATE=en_US.UTF-8 > > x=(-a --b) > > print -r -- ${(on)x} # this prints "-a --b" > > () { > > local LC_ALL= LC_COLLATE=C > > print -r -- ${(on)x} > > } > > print -r -- ${(on)x} # this prints "--b -a" > > ) > > Something like the attached? Slightly but not extensively tested, so I could easily > have missed something. Confirmed it fixes Roman's case (1). When LC_ALL is unset, the patch calls setlocale() for all known locale categories (LC_*), not only for the ones that changed, like langsetfn() and lcsetfn() do. Is this a problem? (I guess there was a reason langsetfn() and lcsetfn() weren't implemented to begin with via the if/else/for combination you just wrote.) I had in mind a different approach: I thought of having scanendscope() call unsetfn for (some subset of?) special parameters and having LANG and LC_* use a custom unsetfn rather than the default one. Even if this is sensible, we can leave it for future work, though; I wouldn't want to make the perfect the enemy of the good. Cheers, Daniel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-03 21:06 ` Daniel Shahaf @ 2020-05-04 8:35 ` Peter Stephenson 2020-05-05 0:03 ` Daniel Shahaf 0 siblings, 1 reply; 37+ messages in thread From: Peter Stephenson @ 2020-05-04 8:35 UTC (permalink / raw) To: Daniel Shahaf, zsh-workers > On 03 May 2020 at 22:06 Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > When LC_ALL is unset, the patch calls setlocale() for all known locale > categories (LC_*), not only for the ones that changed, like langsetfn() > and lcsetfn() do. Is this a problem? (I guess there was a reason > langsetfn() and lcsetfn() weren't implemented to begin with via the > if/else/for combination you just wrote.) I don't think it should be a problem as it's just restoring the current values (unless there's some other bug we're not seeing. It didn't seem to me worthwhile tracking the individual variables when the calls to restore the complete state appear straightforward compared with the overall function exit procedure --- but feel free to disagree if you know more about that than I do as I'm basically just treating it as a black box. pws ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-04 8:35 ` Peter Stephenson @ 2020-05-05 0:03 ` Daniel Shahaf 2020-05-05 16:36 ` Timothée Mazzucotelli 0 siblings, 1 reply; 37+ messages in thread From: Daniel Shahaf @ 2020-05-05 0:03 UTC (permalink / raw) To: zsh-workers Peter Stephenson wrote on Mon, 04 May 2020 09:35 +0100: > > On 03 May 2020 at 22:06 Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > When LC_ALL is unset, the patch calls setlocale() for all known locale > > categories (LC_*), not only for the ones that changed, like langsetfn() > > and lcsetfn() do. Is this a problem? (I guess there was a reason > > langsetfn() and lcsetfn() weren't implemented to begin with via the > > if/else/for combination you just wrote.) > > I don't think it should be a problem as it's just restoring the current > values (unless there's some other bug we're not seeing. It didn't seem > to me worthwhile tracking the individual variables when the calls to > restore the complete state appear straightforward compared with the overall > function exit procedure --- but feel free to disagree if you know more about > that than I do as I'm basically just treating it as a black box. Not disagreeing. Please commit ☺ ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-05 0:03 ` Daniel Shahaf @ 2020-05-05 16:36 ` Timothée Mazzucotelli 2020-05-05 16:47 ` Peter Stephenson 2020-05-05 22:11 ` Bart Schaefer 0 siblings, 2 replies; 37+ messages in thread From: Timothée Mazzucotelli @ 2020-05-05 16:36 UTC (permalink / raw) To: zsh-workers [-- Attachment #1: Type: text/plain, Size: 3760 bytes --] > > I wrote such a test and noticed that file descriptors were being > > closed each time ZSH_XTRACEFD was (re)assigned, even as a local > > variable. So I removed the code lines closing the previous file > > descriptor in xtracefdsetfn, and it seemed to work well. > > This re-introduces the file descriptor leak I was at pains to remove. > It apparently works because the test isn't sensitive to the leak --- > that's hard to fix in a standard way, i.e. with1out having some limit you > can rely on on file descriptors being created. After thinking about it more, do we really want to close the file descriptors? I mean, when assigning ZSH_XTRACEFD, the file descriptor it points to must already exist, i.e. the users must create it themselves, explicitly. We never implicitly create file descriptors for this feature, so why would we implicitly close them? I think closing the file descriptors is the responsibility of the user, just like it's their responsibility to open them in the first place. Besides, the test I mentioned previously was in fact wrong. Closing a file descriptor that a user explicitly created themselves with `exec 6>myfile` just because we left the scope of a function where a local ZSH_XTRACEFD was assigned the value 6 would be quite confusing for the user I think. Example: exec 4>xtrace.log exec 6>xtrace-function.log ZSH_XTRACEFD=4 myfunc() { local ZSH_XTRACEFD=6 print "$1" # do stuff } for i in $(seq 1 10); do myfunc "$i" done In the above example, if we implicitly close the file descriptor 6 because we left `myfunc`'s scope where it was assigned to a local ZSH_XTRACEFD, subsequent calls to `myfunc` will __fail__, because the file descriptor will not be valid anymore. Instead, we should simply let them open, and let the user clean them up later in the script! So, of course file descriptors could leak in the main (interactive) zsh instance, but it would be the user's responsibility entirely, just like currently when doing `exec 7>file7 8>file8` and never closing fds 7 and 8. In fact, ZSH_XTRACEFD has nothing to do with this! In zsh scripts, leaked file descriptors would be removed anyway when the process finishes, and for single commands like `ZSH_XTRACEFD=5 command args 5>myfile`, both the variable and file descriptors are properly cleaned up. Now, if leakage is detected by valgrind when running the test suite, we can simply make sure to close the fds ourselves in every test opening some. If a test uses `exec 9>redir9`, then it should also clean up at the end with `exec 9>&-`. What do you think :) ? Timothée On Tue, May 5, 2020 at 2:04 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > Peter Stephenson wrote on Mon, 04 May 2020 09:35 +0100: > > > On 03 May 2020 at 22:06 Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > When LC_ALL is unset, the patch calls setlocale() for all known locale > > > categories (LC_*), not only for the ones that changed, like langsetfn() > > > and lcsetfn() do. Is this a problem? (I guess there was a reason > > > langsetfn() and lcsetfn() weren't implemented to begin with via the > > > if/else/for combination you just wrote.) > > > > I don't think it should be a problem as it's just restoring the current > > values (unless there's some other bug we're not seeing. It didn't seem > > to me worthwhile tracking the individual variables when the calls to > > restore the complete state appear straightforward compared with the > overall > > function exit procedure --- but feel free to disagree if you know more > about > > that than I do as I'm basically just treating it as a black box. > > Not disagreeing. Please commit ☺ > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-05 16:36 ` Timothée Mazzucotelli @ 2020-05-05 16:47 ` Peter Stephenson 2020-05-05 22:19 ` Bart Schaefer 2020-05-05 22:11 ` Bart Schaefer 1 sibling, 1 reply; 37+ messages in thread From: Peter Stephenson @ 2020-05-05 16:47 UTC (permalink / raw) To: zsh-workers > On 05 May 2020 at 17:36 Timothée Mazzucotelli <timothee.mazzucotelli@gmail.com> wrote: > > > > > I wrote such a test and noticed that file descriptors were being > > > closed each time ZSH_XTRACEFD was (re)assigned, even as a local > > > variable. So I removed the code lines closing the previous file > > > descriptor in xtracefdsetfn, and it seemed to work well. > > > > This re-introduces the file descriptor leak I was at pains to remove. > > It apparently works because the test isn't sensitive to the leak --- > > that's hard to fix in a standard way, i.e. with1out having some limit you > > can rely on on file descriptors being created. > > After thinking about it more, do we really want to close the file > descriptors? > I mean, when assigning ZSH_XTRACEFD, the file descriptor it points to > must already exist, i.e. the users must create it themselves, explicitly. > We never implicitly create file descriptors for this feature, > so why would we implicitly close them? The problem is if we fopen() the file descriptor to use stdio as output, we can either leak the entire FILE, not opened by the user, or we can close the entire FILE. That's unless you can get away with assigning fileno(file) an invalid file descriptor, or there's some other interface I don't know about. I don't think we want to go down the road of not using stdio for stderr, it seems like too much of a waste. pws ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-05 16:47 ` Peter Stephenson @ 2020-05-05 22:19 ` Bart Schaefer 2020-09-03 13:51 ` Timothée Mazzucotelli 0 siblings, 1 reply; 37+ messages in thread From: Bart Schaefer @ 2020-05-05 22:19 UTC (permalink / raw) To: Peter Stephenson; +Cc: zsh-workers (Peter, for some reason Gmail is classifying all email from ntlworld.com as spam, with the notation that it "can't guarantee that this message came from ntlworld.com") On Tue, May 5, 2020 at 9:48 AM Peter Stephenson <p.w.stephenson@ntlworld.com> wrote: > > > The problem is if we fopen() the file descriptor to use stdio as output, we can either > leak the entire FILE, not opened by the user, or we can close the entire FILE. In that case we should be doing the fopen() on a dup() of the descriptor, and fclose()ing the FILE. If it is important that fileno(xtrerr) == $ZSH_XTRACEFD, then we should 1) dup() the descriptor to save a copy 2) fopen() the original 3) after fclose(), dup2() the copy back to the original 4) close() the copy However, I'm not sure it's necessary to be that convoluted. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-05 22:19 ` Bart Schaefer @ 2020-09-03 13:51 ` Timothée Mazzucotelli 2020-09-04 19:48 ` Daniel Shahaf 0 siblings, 1 reply; 37+ messages in thread From: Timothée Mazzucotelli @ 2020-09-03 13:51 UTC (permalink / raw) To: Bart Schaefer; +Cc: Peter Stephenson, zsh-workers [-- Attachment #1.1: Type: text/plain, Size: 1955 bytes --] Hello, I'm coming back to the ZSH_XTRACEFD feature again :) About closing file descriptors or not: I don't understand what it means to "leak a FILE". I'm not sure to understand the past comments either: we only use fdopen when the value of ZSH_XTRACEFD is > 2. For 0, 1 and 2, we re-use the existing file descriptors stdin, stdout and stderr. Anyway, I added the patch in an attachment. Also, here's the link to the commit on my fork: https://github.com/pawamoy/zsh/commit/b9b37333fcf02a463f6f742976b37b45ab08742d In this patch, I never close any file descriptor. There's one last thing that looks weird to me: single instructions like ZSH_XTRACEFD=5 are not properly logged in the xtrace output. It seems they are truncated up to the end of the variable assignment: - with a=0 ZSH_XTRACEFD=5, nothing appear in the output either - with ZXH_XTRACEFD=5 a=0, only a=0 appears in the output (but no +(eval):18 prefix or similar) Any idea about this? Cheers, Timothée On Wed, May 6, 2020 at 12:20 AM Bart Schaefer <schaefer@brasslantern.com> wrote: > (Peter, for some reason Gmail is classifying all email from > ntlworld.com as spam, with the notation that it "can't guarantee that > this message came from ntlworld.com") > > On Tue, May 5, 2020 at 9:48 AM Peter Stephenson > <p.w.stephenson@ntlworld.com> wrote: > > > > > > The problem is if we fopen() the file descriptor to use stdio as output, > we can either > > leak the entire FILE, not opened by the user, or we can close the entire > FILE. > > In that case we should be doing the fopen() on a dup() of the > descriptor, and fclose()ing the FILE. > > If it is important that fileno(xtrerr) == $ZSH_XTRACEFD, then we should > 1) dup() the descriptor to save a copy > 2) fopen() the original > 3) after fclose(), dup2() the copy back to the original > 4) close() the copy > > However, I'm not sure it's necessary to be that convoluted. > [-- Attachment #1.2: Type: text/html, Size: 2855 bytes --] [-- Attachment #2: 0001-44752-Implement-ZSH_XTRACEFD-feature.patch --] [-- Type: text/x-patch, Size: 6497 bytes --] From b9b37333fcf02a463f6f742976b37b45ab08742d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Mazzucotelli?= <pawamoy@pm.me> Date: Thu, 3 Sep 2020 11:27:16 +0200 Subject: [PATCH] 44752: Implement ZSH_XTRACEFD feature --- Src/exec.c | 2 +- Src/init.c | 6 ++-- Src/params.c | 66 +++++++++++++++++++++++++++++++++++++++++- Src/utils.c | 9 +++++- Test/A04redirect.ztst | 67 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 145 insertions(+), 5 deletions(-) diff --git a/Src/exec.c b/Src/exec.c index ecad923de..360dce0ee 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -5453,7 +5453,7 @@ execshfunc(Shfunc shf, LinkList args) cmdsp = 0; if ((osfc = sfcontext) == SFC_NONE) sfcontext = SFC_DIRECT; - xtrerr = stderr; + xtrerr = xtrace_file; doshfunc(shf, args, 0); diff --git a/Src/init.c b/Src/init.c index 3d6c94d04..89c50b17e 100644 --- a/Src/init.c +++ b/Src/init.c @@ -616,8 +616,10 @@ init_io(char *cmd) SHTTY = -1; } - /* Send xtrace output to stderr -- see execcmd() */ - xtrerr = stderr; + /* Send xtrace output to zsh_xtracefd file descriptor -- see execcmd() */ + if (zsh_xtracefd == 0) + zsh_xtracefd = 2; + xtracefdassign(); /* Make sure the tty is opened read/write. */ if (isatty(0)) { diff --git a/Src/params.c b/Src/params.c index 122f5da7d..87208fca3 100644 --- a/Src/params.c +++ b/Src/params.c @@ -106,7 +106,8 @@ zlong lastval, /* $? */ zterm_lines, /* $LINES */ rprompt_indent, /* $ZLE_RPROMPT_INDENT */ ppid, /* $PPID */ - zsh_subshell; /* $ZSH_SUBSHELL */ + zsh_subshell, /* $ZSH_SUBSHELL */ + zsh_xtracefd; /* $ZSH_XTRACEFD */ /* $FUNCNEST */ /**/ @@ -268,6 +269,9 @@ static const struct gsu_array pipestatus_gsu = static const struct gsu_integer rprompt_indent_gsu = { intvargetfn, zlevarsetfn, rprompt_indent_unsetfn }; +static const struct gsu_integer xtracefd_gsu = +{ intvargetfn, xtracefdsetfn, xtracefdunsetfn }; + /* Nodes for special parameters for parameter hash table */ #ifdef HAVE_UNION_INIT @@ -357,6 +361,7 @@ IPDEF5("LINES", &zterm_lines, zlevar_gsu), IPDEF5U("ZLE_RPROMPT_INDENT", &rprompt_indent, rprompt_indent_gsu), IPDEF5("SHLVL", &shlvl, varinteger_gsu), IPDEF5("FUNCNEST", &zsh_funcnest, varinteger_gsu), +IPDEF5("ZSH_XTRACEFD", &zsh_xtracefd, xtracefd_gsu), /* Don't import internal integer status variables. */ #define IPDEF6(A,B,F) {{NULL,A,PM_INTEGER|PM_SPECIAL|PM_DONTIMPORT},BR((void *)B),GSU(F),10,0,NULL,NULL,NULL,0} @@ -4399,6 +4404,65 @@ setsecondstype(Param pm, int on, int off) return 0; } +/* Open / assign the XTRACE fd */ + +/**/ +void xtracefdassign(void) +{ + int fd = (int)zsh_xtracefd; + switch (fd) + { + case 0: /* bizarre, but handle for consistency */ + xtrerr = stdin; + break; + + case 1: + xtrerr = stdout; + break; + + case 2: + xtrerr = stderr; + break; + + default: + xtrerr = fdopen(fd, "w"); + break; + } + xtrace_file = xtrerr; +} + +/* Function to set value of special parameter `ZSH_XTRACEFD' */ + +/**/ +void +xtracefdsetfn(Param pm, zlong fd) +{ + /* Check that the given file descriptor is valid */ + if (fcntl(fd, F_GETFD) != -1 || errno != EBADF) { + intvarsetfn(pm, fd); + xtracefdassign(); + } else + zwarn("file descriptor %d is not valid", fd); + +} + +/* Function to unset value of special parameter `ZSH_XTRACEFD' */ + +/**/ +void +xtracefdunsetfn(Param pm, UNUSED(int exp)) +{ + int current_fd = intvargetfn(pm); + if (current_fd == 2) /* Nothing to do, already using stderr */ + return; + else { /* Reset to file descriptor 2 (stderr) */ + intvarsetfn(pm, 2); + // if (current_fd > 2) + // fclose(xtrerr); /* Never close standard descriptors */ + xtrerr = xtrace_file = stderr; + } +} + /* Function to get value for special parameter `USERNAME' */ /**/ diff --git a/Src/utils.c b/Src/utils.c index 5151b89a8..efe28c9c6 100644 --- a/Src/utils.c +++ b/Src/utils.c @@ -1760,12 +1760,19 @@ checkmailpath(char **s) /**/ FILE *xtrerr = 0; +/* This records the last file XTRACE was open too. + * It's used for restoring XTRACE after a possible redirection. + */ + +/**/ +FILE *xtrace_file; + /**/ void printprompt4(void) { if (!xtrerr) - xtrerr = stderr; + xtracefdassign(); if (prompt4) { int l, t = opts[XTRACE]; char *s = dupstring(prompt4); diff --git a/Test/A04redirect.ztst b/Test/A04redirect.ztst index 993138e7d..1e34d4961 100644 --- a/Test/A04redirect.ztst +++ b/Test/A04redirect.ztst @@ -722,3 +722,70 @@ >Works >Works ?(eval):6: file exists: foo + + rm -f redir + set -x + ZSH_XTRACEFD=4 print 'This is ZSH_XTRACEFD redir' 4>redir + set +x + cat redir +0:Redirect xtrace output to ZSH_XTRACEFD file descriptor +>This is ZSH_XTRACEFD redir +>+(eval):3> print 'This is ZSH_XTRACEFD redir' +?+(eval):3> ZSH_XTRACEFD=4 +(eval):4> set +x + + rm -f redir + A() { + local ZSH_XTRACEFD=5 + B + print 'Function A to file descriptor 5' + unset ZSH_XTRACEFD + print 'Function A to file descriptor 2' + } + B() { + local ZSH_XTRACEFD=6 + print 'Function B to file descriptor 6' + } + exec 4>redir4 5>redir5 6>redir6 + ZSH_XTRACEFD=4 + set -x + print 'Main to file descriptor 4' + A + print 'Main to file descriptor 4 again\n' + a=0 ZSH_XTRACEFD=5 # appears as blank line in redir5 + ZSH_XTRACEFD=6 # appears as blank line in redir6 + unset ZSH_XTRACEFD + set +x + print "end of file redir4" >> redir4 + cat redir4 + print + print "end of file redir5" >> redir5 + cat redir5 + print + print "end of file redir6" >> redir6 + cat redir6 +0:Scoped ZSH_XTRACEFD correctly set and restored +>Main to file descriptor 4 +>Function B to file descriptor 6 +>Function A to file descriptor 5 +>Function A to file descriptor 2 +>Main to file descriptor 4 again +> +>+(eval):16> print 'Main to file descriptor 4' +>+(eval):17> A +>+A:1> local ZSH_XTRACEFD=5 +>+(eval):18> print 'Main to file descriptor 4 again\n' +>end of file redir4 +> +>+A:2> B +>+B:1> local ZSH_XTRACEFD=6 +>+A:3> print 'Function A to file descriptor 5' +>+A:4> unset ZSH_XTRACEFD +> +>end of file redir5 +> +>+B:2> print 'Function B to file descriptor 6' +> +>+(eval):21> unset ZSH_XTRACEFD +>end of file redir6 +?+A:5> print 'Function A to file descriptor 2' +?+(eval):22> set +x -- 2.28.0 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-09-03 13:51 ` Timothée Mazzucotelli @ 2020-09-04 19:48 ` Daniel Shahaf 2020-09-04 19:53 ` Bart Schaefer 0 siblings, 1 reply; 37+ messages in thread From: Daniel Shahaf @ 2020-09-04 19:48 UTC (permalink / raw) To: Timothée Mazzucotelli; +Cc: Bart Schaefer, Peter Stephenson, zsh-workers Timothée Mazzucotelli wrote on Thu, 03 Sep 2020 15:51 +0200: > Hello, I'm coming back to the ZSH_XTRACEFD feature again :) > > About closing file descriptors or not: I don't understand what it means to > "leak a FILE". "FILE", in this context, is the opaque type defined by the C programming language. (See the return type of fopen(3), for example.) In implementations, a FILE typically wraps a "file descriptor" (fd), which is an int. The kernel caps the number of file descriptors the process may have open at any one time (see ulimit/unlimit). For this reason it's important to always close fd's when done using them. To "leak" a fd means to lose track of its integer value and the fact it needs to be closed. For example, the following function leaks a fd: . static void f(void) { open("/dev/null", O_WRONLY); return; } Every call to f() leaks one fd. If f() were called in a loop, eventually it would not be possible to open more files: . [[[ % repeat $((1 + $(ulimit -n))) { eval "exec {fd$((++i))}>/dev/null" } zsh: cannot moved fd 3: too many open files zsh: exit 1 ⋮ ]]] (That's rather dense, I'm afraid, so feel free to ask further. The number of iterations is large enough to ensure the error happens in the last iteration or earlier. The variable $i is autovivified and used to generate a different variable name in every iteration of the loop ($fd1, $fd2, …, $fd1025) since I didn't know whether the square brackets array element variable name syntax could be used in that context.) If f() called fopen() rather than open(), it would leak not only a fd but also whatever other resources the FILE encapsulated (e.g., a small block of malloc()'d memory). HTH. I'll leave the ZSH_XTRACEFD-specific aspects for others. Cheers, Daniel P.S. The quoted error message is ungrammatical. > I'm not sure to understand the past comments either: we only use fdopen > when the value of ZSH_XTRACEFD is > 2. > For 0, 1 and 2, we re-use the existing file descriptors stdin, stdout and > stderr. > > Anyway, I added the patch in an attachment. Also, here's the link to the > commit on my fork: > https://github.com/pawamoy/zsh/commit/b9b37333fcf02a463f6f742976b37b45ab08742d > > In this patch, I never close any file descriptor. > > There's one last thing that looks weird to me: > single instructions like ZSH_XTRACEFD=5 are not properly logged in the > xtrace output. > It seems they are truncated up to the end of the variable assignment: > - with a=0 ZSH_XTRACEFD=5, nothing appear in the output either > - with ZXH_XTRACEFD=5 a=0, only a=0 appears in the output (but no > +(eval):18 prefix or similar) > > Any idea about this? > > Cheers, > Timothée > > On Wed, May 6, 2020 at 12:20 AM Bart Schaefer <schaefer@brasslantern.com> > wrote: > > > (Peter, for some reason Gmail is classifying all email from > > ntlworld.com as spam, with the notation that it "can't guarantee that > > this message came from ntlworld.com") > > > > On Tue, May 5, 2020 at 9:48 AM Peter Stephenson > > <p.w.stephenson@ntlworld.com> wrote: > > > > > > > > > The problem is if we fopen() the file descriptor to use stdio as output, > > we can either > > > leak the entire FILE, not opened by the user, or we can close the entire > > FILE. > > > > In that case we should be doing the fopen() on a dup() of the > > descriptor, and fclose()ing the FILE. > > > > If it is important that fileno(xtrerr) == $ZSH_XTRACEFD, then we should > > 1) dup() the descriptor to save a copy > > 2) fopen() the original > > 3) after fclose(), dup2() the copy back to the original > > 4) close() the copy > > > > However, I'm not sure it's necessary to be that convoluted. > > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-09-04 19:48 ` Daniel Shahaf @ 2020-09-04 19:53 ` Bart Schaefer 2020-09-05 9:24 ` Timothée Mazzucotelli 0 siblings, 1 reply; 37+ messages in thread From: Bart Schaefer @ 2020-09-04 19:53 UTC (permalink / raw) To: zsh-workers On Fri, Sep 4, 2020 at 12:49 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > zsh: cannot moved fd 3: too many open files > > P.S. The quoted error message is ungrammatical. That is supposed to say "cannot movefd fd 3" ... movefd being the name of the function ... someone has done a spell-checking pass over the error messages and fixed a "mistake" that wasn't actually a mistake? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-09-04 19:53 ` Bart Schaefer @ 2020-09-05 9:24 ` Timothée Mazzucotelli 0 siblings, 0 replies; 37+ messages in thread From: Timothée Mazzucotelli @ 2020-09-05 9:24 UTC (permalink / raw) To: zsh-workers [-- Attachment #1: Type: text/plain, Size: 1972 bytes --] Thank you very much Daniel for this great explanation on what leaking a FILE (or a fd) means. It's much clearer now. From what I understand, the code for the ZSH_XTRACEFD feature will then not leak any file descriptor itself. Based on the man pages: > The *fdopen*() function associates a stream with the existing file descriptor, *fd*. [...] > The file descriptor is not dup'ed, and will be closed when the stream created by *fdopen*() is closed. The user is the one creating the file descriptor initially, and all Zsh does is assigning it to the xtrerr variable. > xtrerr = fdopen(fd, "w"); Zsh should not close the file descriptor itself. As stated previously, it should be the user's choice and responsibility to close the file descriptors they have opened. If, however, the stream associated to the fd by fdopen allocates memory, then not closing that stream and assigning another value to xtrerr would indeed cause a memory leak. In that case, and now that I understand Bart's answer under the light of your explanation, we could fdopen a duplicate of the fd instead, so we can close both the duplicated fd and its associated stream, fixing the memory leak. We could use the valgrind test suite to make sure there's a memory leak or not, unfortunately I did not manage to have it working on my computer (there were a lot of reported leaks when there should not have been any, see previous messages). Timothée On Fri, Sep 4, 2020 at 9:54 PM Bart Schaefer <schaefer@brasslantern.com> wrote: > On Fri, Sep 4, 2020 at 12:49 PM Daniel Shahaf <d.s@daniel.shahaf.name> > wrote: > > > > zsh: cannot moved fd 3: too many open files > > > > P.S. The quoted error message is ungrammatical. > > That is supposed to say "cannot movefd fd 3" ... movefd being the name > of the function ... someone has done a spell-checking pass over the > error messages and fixed a "mistake" that wasn't actually a mistake? > > [-- Attachment #2: Type: text/html, Size: 3129 bytes --] ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-05 16:36 ` Timothée Mazzucotelli 2020-05-05 16:47 ` Peter Stephenson @ 2020-05-05 22:11 ` Bart Schaefer 1 sibling, 0 replies; 37+ messages in thread From: Bart Schaefer @ 2020-05-05 22:11 UTC (permalink / raw) To: Timothée Mazzucotelli; +Cc: zsh-workers [-- Attachment #1: Type: text/plain, Size: 576 bytes --] On Tue, May 5, 2020 at 9:37 AM Timothée Mazzucotelli < timothee.mazzucotelli@gmail.com> wrote: > After thinking about it more, do we really want to close the file > descriptors? > I mean, when assigning ZSH_XTRACEFD, the file descriptor it points to > must already exist, i.e. the users must create it themselves, explicitly. > We never implicitly create file descriptors for this feature, > so why would we implicitly close them? > Excellent point. Explicitly opened descriptors should be explicitly closed, except in some special cases of forked subshells. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-03 0:06 ` Daniel Shahaf 2020-05-03 4:43 ` Roman Perepelitsa @ 2020-05-03 6:01 ` Stephane Chazelas 2020-05-03 7:07 ` Stephane Chazelas 1 sibling, 1 reply; 37+ messages in thread From: Stephane Chazelas @ 2020-05-03 6:01 UTC (permalink / raw) To: Daniel Shahaf Cc: Bart Schaefer, Timothée Mazzucotelli, Peter Stephenson, zsh-workers 2020-05-03 00:06:58 +0000, Daniel Shahaf: [...] > P.S. Shouldn't ZSH_XTRACEFD be declared with PM_DONTIMPORT? (assuming PM_DONTIMPORT is about not importing it from the environment) ZSH_XTRACEFD is useful in things like: ZSH_XTRACEFD=7 zsh -x myscript 7> somefile Or ZSH_XTRACEFD=7 zsh -lx 7> somefile to debug start-up file issues. That's the primary way I've been using its bash counterpart. See https://unix.stackexchange.com/search?q=user%3A22565+BASH_XTRACEFD for several examples. bash also allows SHELLOPTS=xtrace BASH_XTRACEFD=7 some-command Which will cause all bash invocations run through some-command (including those running as sh!) to have their xtrace option set. I've used that to debug things like grub-install. I don't think we want to go there with zsh. With zsh, one can always add a: if [[ -n $MYDEBUG ]]; then ZSH_XTRACEFD=7 set -o xtrace fi to their ~/.zshenv to enable this kind of debugging. -- Stephane ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-03 6:01 ` Stephane Chazelas @ 2020-05-03 7:07 ` Stephane Chazelas 0 siblings, 0 replies; 37+ messages in thread From: Stephane Chazelas @ 2020-05-03 7:07 UTC (permalink / raw) To: Daniel Shahaf, Bart Schaefer, Timothée Mazzucotelli, Peter Stephenson, zsh-workers 2020-05-03 07:01:13 +0100, Stephane Chazelas: [...] > SHELLOPTS=xtrace BASH_XTRACEFD=7 some-command [...] > I don't think we want to go there with zsh. [...] I meant: we probably don't want to have options set via the environment like with bash's SHELLOPTS and BASHOPTS, as that's quite dangerous and does cause quite a few problems with bash See for instance: $ (SHELLOPTS= exec -a sh bash -c 'bash -c "printenv SHELLOPTS"') braceexpand:hashall:interactive-comments:posix See how all those POSIX compliance options have been enabled for *all* bash invocations, just because bash was invoked as sh in an environment with a SHELLOPTS variable. But for ZSH_XTRACEFD, IMO it makes sense to import it as we do already import PS4. Note that BASHOPTS, SHELLOPTS and PS4 are in sudo's env var blacklist, but BASH_XTRACEFD is not (maybe it should) -- Stephane ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-02 18:02 ` Timothée Mazzucotelli 2020-05-02 21:15 ` Bart Schaefer @ 2020-05-03 17:30 ` Peter Stephenson 2020-05-03 21:23 ` Daniel Shahaf 1 sibling, 1 reply; 37+ messages in thread From: Peter Stephenson @ 2020-05-03 17:30 UTC (permalink / raw) To: zsh-workers On Sat, 2020-05-02 at 20:02 +0200, Timothée Mazzucotelli wrote: > I wrote such a test and noticed that file descriptors were being > closed each time ZSH_XTRACEFD was (re)assigned, even as a local > variable. So I removed the code lines closing the previous file > descriptor in xtracefdsetfn, and it seemed to work well. This re-introduces the file descriptor leak I was at pains to remove. It apparently works because the test isn't sensitive to the leak --- that's hard to fix in a standard way, i.e. with1out having some limit you can rely on on file descriptors being created. The key to this is the same as the point Daniel made: there needs to be special handling in parameter start/end scope for local variables. There are other variables that have some prior art for this --- e.g. SECONDS has special handling to make sure we get back the prevoius starting point for times. This variable might want something similar to make sure we remember the previous file descriptor rather than close it, but also make sure we close a local file descriptor on exit. It might be possible to detect in the parameter-specific functions that a new file descriptor is being called locally and the old one needs saving in a way it can be restored, but that might be harder than special code in the parmater scope hanling which should at least pick up all cases. As you'll see I haven't really thought this through at the moment... pws ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-03 17:30 ` Peter Stephenson @ 2020-05-03 21:23 ` Daniel Shahaf 2020-05-04 8:26 ` Peter Stephenson 0 siblings, 1 reply; 37+ messages in thread From: Daniel Shahaf @ 2020-05-03 21:23 UTC (permalink / raw) To: zsh-workers Peter Stephenson wrote on Sun, 03 May 2020 18:30 +0100: > On Sat, 2020-05-02 at 20:02 +0200, Timothée Mazzucotelli wrote: > > I wrote such a test and noticed that file descriptors were being > > closed each time ZSH_XTRACEFD was (re)assigned, even as a local > > variable. So I removed the code lines closing the previous file > > descriptor in xtracefdsetfn, and it seemed to work well. > > This re-introduces the file descriptor leak I was at pains to remove. > It apparently works because the test isn't sensitive to the leak --- > that's hard to fix in a standard way, i.e. with1out having some limit you > can rely on on file descriptors being created. > Which test is that? Cheers, Daniel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-05-03 21:23 ` Daniel Shahaf @ 2020-05-04 8:26 ` Peter Stephenson 0 siblings, 0 replies; 37+ messages in thread From: Peter Stephenson @ 2020-05-04 8:26 UTC (permalink / raw) To: zsh-workers > On 03 May 2020 at 22:23 Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > Peter Stephenson wrote on Sun, 03 May 2020 18:30 +0100: > > On Sat, 2020-05-02 at 20:02 +0200, Timothée Mazzucotelli wrote: > > > I wrote such a test and noticed that file descriptors were being > > > closed each time ZSH_XTRACEFD was (re)assigned, even as a local > > > variable. So I removed the code lines closing the previous file > > > descriptor in xtracefdsetfn, and it seemed to work well. > > > > This re-introduces the file descriptor leak I was at pains to remove. > > It apparently works because the test isn't sensitive to the leak --- > > that's hard to fix in a standard way, i.e. with1out having some limit you > > can rely on on file descriptors being created. > > > > Which test is that? I'm thinking of 45760, but actually it's explicitly testing for closed file descriptors (rather than looking for a leak), so that should be on the right lines. I'll look a bit more closely to check what's going on when I get a chance --- I'm evidently missing aspects of how this fits together anyway. pws ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Feature request: ZSH_XTRACEFD variable 2020-04-19 10:30 ` Timothée Mazzucotelli 2020-04-20 14:09 ` Peter Stephenson @ 2020-05-05 20:50 ` Daniel Shahaf 1 sibling, 0 replies; 37+ messages in thread From: Daniel Shahaf @ 2020-05-05 20:50 UTC (permalink / raw) To: Timothée Mazzucotelli; +Cc: Peter Stephenson, zsh-workers Timothée Mazzucotelli wrote on Sun, 19 Apr 2020 12:30 +0200: > I managed to write a test for the ZSH_XTRACEFD feature in A04redirect.ztst, > and am planning to add more tests, but maybe you'd have some hints on what > I should test? The case that a shadowed value is equal to the value that's going out of scope: ZSH_XTRACEFD=3 () { local ZSH_XTRACEFD=2 () { local ZSH_XTRACEFD=3 } # at this point fd=3 should still be open } ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2020-09-05 9:25 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-17 15:08 Feature request: ZSH_XTRACEFD variable Timothée Mazzucotelli 2019-05-18 7:55 ` Stephane Chazelas 2019-05-20 10:34 ` Stephane Chazelas 2019-07-21 15:08 ` Timothée Mazzucotelli 2019-07-21 15:22 ` Peter Stephenson 2019-07-31 19:40 ` Timothée Mazzucotelli 2019-07-31 19:45 ` Timothée Mazzucotelli 2019-07-31 21:12 ` Sebastian Gniazdowski 2019-08-13 9:14 ` Peter Stephenson 2019-08-13 15:38 ` Peter Stephenson 2019-09-10 22:19 ` Timothée Mazzucotelli 2019-09-11 8:45 ` Peter Stephenson 2020-04-19 10:30 ` Timothée Mazzucotelli 2020-04-20 14:09 ` Peter Stephenson 2020-05-02 18:02 ` Timothée Mazzucotelli 2020-05-02 21:15 ` Bart Schaefer 2020-05-03 0:06 ` Daniel Shahaf 2020-05-03 4:43 ` Roman Perepelitsa 2020-05-03 16:21 ` Daniel Shahaf 2020-05-03 19:54 ` Peter Stephenson 2020-05-03 21:06 ` Daniel Shahaf 2020-05-04 8:35 ` Peter Stephenson 2020-05-05 0:03 ` Daniel Shahaf 2020-05-05 16:36 ` Timothée Mazzucotelli 2020-05-05 16:47 ` Peter Stephenson 2020-05-05 22:19 ` Bart Schaefer 2020-09-03 13:51 ` Timothée Mazzucotelli 2020-09-04 19:48 ` Daniel Shahaf 2020-09-04 19:53 ` Bart Schaefer 2020-09-05 9:24 ` Timothée Mazzucotelli 2020-05-05 22:11 ` Bart Schaefer 2020-05-03 6:01 ` Stephane Chazelas 2020-05-03 7:07 ` Stephane Chazelas 2020-05-03 17:30 ` Peter Stephenson 2020-05-03 21:23 ` Daniel Shahaf 2020-05-04 8:26 ` Peter Stephenson 2020-05-05 20:50 ` Daniel Shahaf
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).