Re: [RFC PATCH 2/2] reftable: don't memset() a NULL from failed malloc()

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2022, 16:23 UTC
Message-ID: <xmqq1qxyfh8k.fsf@gitster.g>
In-Reply-To: <220415.86bkx2bb0p.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 9 quoted lines

>> Does it? I just quickly scanned the output of
>>
>> git grep -e 'reftable_[mc]alloc' -e 'reftable_realloc' -A2 origin/master
>>
>> and I didn't see a single NULL check
>
> I think you're right, I retrieved that information from wetware. Looking
> again I think I was confusing it with the if (x) free(x) fixes in
> 34230514b83 (Merge branch 'hn/reftable-coverity-fixes', 2022-02-16).

True. Initially I hoped that these RFC changes gives us a better solution that comes from stepping back and rethinking the issues around the original "why are we calling memset() with NULL?", and if it were only "well, in _return_block() functions, we clear the block before calling _free()---that shouldn't be necessary, if the calling custom malloc-free pair cares, they can do the clearing, and plain vanilla free() certainly doesn't, so let's stop calling memset()", it certainly would have fallen into that category, but everything these RFC patches do beyond that seems "oh, it does not look important to me, so let's rip it out to simplify", without knowing enough to say if that is sensible.

But ...

Show 6 quoted lines

>> Isn't it there so the same code can be used by libgit2 and other
>> things that let the caller specify a custom allocator?
>
> I don't think so, but I may be wrong. I think it's a case of
> over-generalization where we'd be better off with something simpler,
> i.e. just using our normal allocation functions.

... many points that was raised on the reftable code in this thread may deserve response to explain the rationale behind the current code and design, as nobody can improve, without breaking the intended way to be used, without knowing what it is. Thanks for marking the patches with RFC. I think the "patch" is a bit too dense a form to point out the issues in the existing code, but the discussion that follows by quoting the points in the original code and suggested changes is a good way to think about the code before these RFC patches.

Ideally, it would have been much better if these points were raised during the review before the code was accepted to the code base, but it is better to have one now, rather than never ;-)

THanks.