Re: Feature request: ZSH_XTRACEFD variable

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

> > 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@xxxxxxxxxxxxxxxxxx> wrote:

> Peter Stephenson wrote on Mon, 04 May 2020 09:35 +0100:
> > > On 03 May 2020 at 22:06 Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> 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 ☺
>