On Wed, Nov 25, 2020 at 08:04:44PM -0500, Taylor Blau wrote: > > So we could perhaps BUG() in find_revindex_entry() instead of returning > > NULL. A quick segfault accomplishes mostly the same thing, though the > > BUG() could distinguish the two cases more clearly. > > Yeah, a find_revindex_entry() that returns NULL means that the caller is > probably dead in the water. > > FWIW, this function gets touched by a series that I'm working on here: > [1]. There, I think "returning NULL" is equivalent to "returning -1", > and the problem exists there, too. > > We could return a different negative number, call BUG(), or do nothing > other than what's written. I don't have any strong feelings, though. Yeah, I was suggesting _never_ returning a failure, and just hitting a BUG() within the function. So it does not matter then how you represent the error return type, because there isn't one. :) Looking over the callers, there are actually a few that check the return value and handle it sanely. I suspect they can never trigger in practice, given that the other callers would all segfault, and we have not seen any reports in the 15 years during which that has been the case. But perhaps some of them can be triggered by bogus pack data that nobody has ever run into in the real world. At any rate, I am content to ignore it until somebody feels like digging into each caller. A BUG() is only marginally better than an immediate segfault anyway, and I'd prefer not to disrupt more substantive improvements in the area. -Peff
