Re: [PATCH] pack-revindex.c: don't close unopened file descriptors

From: Jeff King <peff@peff.net>
Date: Feb 26, 2021, 17:55 UTC
Message-ID: <YDk1/y1CmsPYC88C@coredump.intra.peff.net>
In-Reply-To: <34c017296a7c5fe4a2ea70f8a0b2d8586b34a4dc.1614357030.git.me@ttaylorr.com>

On Fri, Feb 26, 2021 at 11:31:02AM -0500, Taylor Blau wrote:

Show 10 quoted lines

> When opening a reverse index, load_revindex_from_disk() jumps to the
> 'cleanup' label in case something goes wrong: the reverse index had the
> wrong size, an unrecognized version, or similar.
> 
> It also jumps to this label when the reverse index couldn't be opened in
> the first place, which will cause an error with the unguarded close()
> call in the label.
> 
> Guard this call with "if (fd >= 0)" to make sure that we have a valid
> file descriptor to close before attempting to close it.

Makes sense. Running close(-1) wasn't really hurting much in practice, but it's cleaner not to do so.

This would go on top of tb/pack-revindex-on-disk.

> Dscho mentioned this to me privately when reviewing Coverity results for
> -rc0. This one is legitimate, and the fix is easy enough, too.

I'm excited that we might get Coverity results again. There were a lot of false positives, but I found its signal-to-noise ratio higher than almost every other static analysis tool I've looked at.

-Peff