zsh-workers
 help / color / mirror / code / Atom feed
* 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  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-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-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  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 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-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-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

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

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