Re: Feature request: ZSH_XTRACEFD variable

[Timothée Mazzucotelli <timothee.mazzucotelli@xxxxxxxxx>]

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@xxxxxxxxx> 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@xxxxxxxxxxx>
> 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
>>
>>