Re: [PATCH] csum-file: introduce discard_hashfile()

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2181 bytes --]

On Fri, Jul 26, 2024 at 12:42:16AM -0400, Jeff King wrote:
> On Thu, Jul 25, 2024 at 04:07:28PM -0700, Junio C Hamano wrote:
> 
> > Introduce discard_hashfile() API function to allow them to release
> > the resources held by a hashfile structure the callers want to
> > dispose of, and use that in read-cache.c:do_write_index(), which is
> > a central place that writes the index file.
> 
> Nicely explained, and the patch looks good to me.
> 
> A few small comments (that probably do not need any changes):
> 
> > +void discard_hashfile(struct hashfile *f)
> > +{
> > +	if (0 <= f->check_fd)
> > +		close(f->check_fd);
> > +	if (0 <= f->fd)
> > +		close(f->fd);
> > +	free_hashfile(f);
> > +}

Are we sure that this is always correct? A valid file descriptor may
have a zero value, and we wouldn't end up closing it here.

> > @@ -2992,8 +2992,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
> >  		hashwrite(f, sb.buf, sb.len);
> >  		strbuf_release(&sb);
> >  		free(ieot);
> > -		if (err)
> > -			return -1;
> > +		/*
> > +		 * NEEDSWORK: write_index_ext_header() never returns a failure,
> > +		 * and this part may want to be simplified.
> > +		 */
> > +		if (err) {
> > +			err = -1;
> > +			goto discard_hashfile_and_return;
> > +		}
> >  	}
> 
> There's other repeated cleanup happening here, like free(ieot) and
> strbuf_release(), which made wonder if we could bump it down to the
> cleanup label at the end of the function to simplify things. But
> probably not, as we are often doing that cleanup even in the non-error
> case. And of course the "sb" strbuf is local to a lot of blocks.
> 
> So even if we did want to do it, I think it would come as a separate
> patch. But mostly I wondered whether the label should be a more generic
> "cleanup" than "discard_hashfile". We could probably worry about that
> later, though, if that separate patch ever materializes.

Indeed, I wanted to say the same. I've got a patch series sitting around
locally where I do this. I guess I should send out my memory leak fixes
sooner rather than later to avoid duplicated work :)

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]