From: Eric Sunshine <sunshine@sunshineco.com>
To: Jeff King <peff@peff.net>
Cc: Hoyoung Lee <lhywkd22@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup
Date: Wed, 23 Jul 2025 12:37:16 -0400 [thread overview]
Message-ID: <CAPig+cSzYPaBGZ_muhxwcNy4xWaMF5Uc0+SN7ygnS4wNaGxEKQ@mail.gmail.com> (raw)
In-Reply-To: <20250723084624.GA580010@coredump.intra.peff.net>
On Wed, Jul 23, 2025 at 4:46 AM Jeff King <peff@peff.net> wrote:
> On Wed, Jul 23, 2025 at 04:11:26AM -0400, Eric Sunshine wrote:
> > For completeness, I'll mention that I even had the thought that
> > another "fix" would be to tear out all the cleanup code entirely since
> > we _know_ that this function will be exiting immediately and the OS
> > will clean up any dangling resources.
>
> The reason we have the "cleanup" label at all is because of the memory
> leaks. And there the issue is that we build the test helpers with the
> same compiler settings as the rest of the code, so SANITIZE=leak will
> complain. So I think that is a non-starter.
>
> But if you just meant leaking descriptors, sure, I don't think any tools
> complain about that. ;)
At the time the momentary (but not really serious) thought flashed
through my brain, I probably was imagining dropping all of the cleanup
code; I almost certainly wasn't thinking about the SANITIZE=leak case.
But immediately upon seeing your idea to use die(), I recognized it
(and liked it) as a superior version of my half-baked thought.
next prev parent reply other threads:[~2025-07-23 16:37 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-07-22 17:41 [PATCH v3 0/2] fix resource leaks in test helpers Hoyoung Lee
2025-07-22 17:41 ` [PATCH v3 1/2] t/helper/test-truncate: close file descriptor after truncation Hoyoung Lee
2025-07-22 21:48 ` Junio C Hamano
2025-07-22 17:41 ` [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup Hoyoung Lee
2025-07-23 7:28 ` Eric Sunshine
2025-07-23 7:55 ` Jeff King
2025-07-23 8:06 ` Jeff King
2025-07-23 8:17 ` Eric Sunshine
2025-07-23 23:59 ` Jeff King
2025-07-24 0:00 ` [PATCH 1/3] test-delta: handle errors with die() Jeff King
2025-07-24 0:02 ` [PATCH 2/3] test-delta: use strbufs to hold input files Jeff King
2025-07-24 0:03 ` [PATCH 3/3] test-delta: close output descriptor after use Jeff King
2025-07-23 8:11 ` [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup Eric Sunshine
2025-07-23 8:46 ` Jeff King
2025-07-23 16:37 ` Eric Sunshine [this message]
2025-07-23 17:11 ` Junio C Hamano
2025-07-23 16:48 ` Junio C Hamano
2025-07-23 16:44 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPig+cSzYPaBGZ_muhxwcNy4xWaMF5Uc0+SN7ygnS4wNaGxEKQ@mail.gmail.com \
--to=sunshine@sunshineco.com \
--cc=git@vger.kernel.org \
--cc=lhywkd22@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).