zsh-workers
 help / 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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
  2 siblings, 1 reply; 12+ 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] 12+ 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
  0 siblings, 1 reply; 12+ 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] 12+ 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; 12+ 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] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ 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

zsh-workers

Archives are clonable: git clone --mirror http://inbox.vuxu.org/zsh-workers

Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.zsh.workers


AGPL code for this site: git clone https://public-inbox.org/ public-inbox