From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=DKIM_ADSP_CUSTOM_MED, FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.2 Received: (qmail 20669 invoked from network); 5 May 2020 16:37:48 -0000 Received-SPF: pass (primenet.com.au: domain of zsh.org designates 203.24.36.2 as permitted sender) receiver=inbox.vuxu.org; client-ip=203.24.36.2 envelope-from= Received: from ns1.primenet.com.au (HELO primenet.com.au) (203.24.36.2) by inbox.vuxu.org with ESMTPUTF8; 5 May 2020 16:37:48 -0000 Received: (qmail 684 invoked by alias); 5 May 2020 16:37:35 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: List-Unsubscribe: X-Seq: 45783 Received: (qmail 8185 invoked by uid 1010); 5 May 2020 16:37:35 -0000 X-Qmail-Scanner-Diagnostics: from mail-lj1-f175.google.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.102.2/25801. spamassassin: 3.4.4. Clear:RC:0(209.85.208.175):SA:0(-2.0/5.0):. Processed in 2.546031 secs); 05 May 2020 16:37:35 -0000 X-Envelope-From: timothee.mazzucotelli@gmail.com X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | Received-SPF: pass (ns1.primenet.com.au: SPF record at _netblocks.google.com designates 209.85.208.175 as permitted sender) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=H7NXMsMZAu8IejRl8rb3ffwczabgImvBN8xceXfFaz4=; b=gDDF5pOuq1PQF55mou2aUPBQt9zhFLTYrVeb8Nrvj+68NEKFfIj7CMBXR+SBwUUzXI RYxV7OtIXkgLrVEwPe+Jr+wVV4CiaZj8hwwaHkk5DtNT7L9OitLio3W5EgXYdWQSiQbT dwN/cAaTVCKShaoIg7rZRw7d1Jr1UJkHxJ7sN9DsBgKxwYZOMcDrB1JTC7vH4PYottXy XDn3bEl6jpSrR4+kDXuzkx9WGxbNDWKtl7dTD/QoL8IXM6bUO7TG25vrj3BnFDvlQSDe lr1Bl11YoYQW7oj16ruAJr/UTyfjKYmHsmWqkE59CV13v9Jzi7FTCnuXVnM7ylALK5M8 XJBw== X-Gm-Message-State: AGi0Pubr7M8D63kppxv0BNm7FBCdoBbYXQQ/mz3IVSu8Ekndky36WnnZ F7sJ4QmIM7pn+e4Hl9ewPWFqSG8avqIdRBVLI3pBUEuRa6I= X-Google-Smtp-Source: APiQypLMOJ0Ykb3T0tIeniPs1npU4enhVJVAcokq8NXlR0IHHpklYEScfrOFpMMHr1YdKbEfeCm99ZiUaiOevoz8tqo= X-Received: by 2002:a2e:760c:: with SMTP id r12mr2257815ljc.139.1588696617232; Tue, 05 May 2020 09:36:57 -0700 (PDT) MIME-Version: 1.0 References: <1563722540.4311.24.camel@samsung.com> <1565710707.5633.11.camel@samsung.com> <309829031.4459446.1587391766024@mail2.virginmedia.com> <20200503000658.6fddb904@tarpaulin.shahaf.local2> <20200503210618.5c639014@tarpaulin.shahaf.local2> <505277422.148264.1588581302888@mail2.virginmedia.com> <20200505000331.59294412@tarpaulin.shahaf.local2> In-Reply-To: <20200505000331.59294412@tarpaulin.shahaf.local2> From: =?UTF-8?Q?Timoth=C3=A9e_Mazzucotelli?= Date: Tue, 5 May 2020 18:36:45 +0200 Message-ID: Subject: Re: Feature request: ZSH_XTRACEFD variable To: zsh-workers@zsh.org Content-Type: multipart/alternative; boundary="00000000000086b04805a4e9417a" --00000000000086b04805a4e9417a Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable > > 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=3D4 myfunc() { local ZSH_XTRACEFD=3D6 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=3D5 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=C3=A9e On Tue, May 5, 2020 at 2:04 AM Daniel Shahaf wrote= : > Peter Stephenson wrote on Mon, 04 May 2020 09:35 +0100: > > > On 03 May 2020 at 22:06 Daniel Shahaf wrote: > > > When LC_ALL is unset, the patch calls setlocale() for all known local= e > > > 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 =E2=98=BA > --00000000000086b04805a4e9417a--