On Wed, Jul 23, 2025 at 3:55 AM Jeff King <peff@xxxxxxxx> wrote: > On Wed, Jul 23, 2025 at 03:28:05AM -0400, Eric Sunshine wrote: > > The descriptor is closed manually (again) because a subsequent open() > > call is going to reuse the variable. However... > > ...although `fd` was closed, it still holds the previously-open > > non-negative file descriptor, which means that this `goto cleanup`... > > Oof, good catch. This iteration of the patch was based on my suggestion, > but I didn't notice the jump to cleanup between that close/open pair. > > I dunno. We are reaching diminishing returns spending brainpower on a > function that is meant to be somewhat quick-and-dirty. Aside from preferring that the patch not make the code worse or more confusing, I don't have a strong opinion. Superficially, the existing code presents itself as being careful by cleaning up after itself, but as Hoyoung Lee discovered, there are holes in the cleanup implementation. So, I do like that the intention of the patch is to plug those holes, but we don't necessarily need to be particularly clever about it. For such simple test-related code, even a pure brute-force fix should be acceptable. 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.
