Re: Feature request: ZSH_XTRACEFD variable

["Timothée Mazzucotelli" <timothee.mazzucotelli@gmail.com>]

[-- 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);
>