Re: [PATCH] khash: clarify that allocations never fail

[Jeff King <peff@peff.net>]

On Sat, Jul 03, 2021 at 02:56:50PM +0200, René Scharfe wrote:

> > If anything we might consider renaming it via coccinelle to
> > XALLOC_ARRAY(), XREALLOC_ARRAY() etc. to make it clear that they handle
> > any errors themselves.
> 
> I don't think there's any confusion in our internal code about the macros'
> handling of allocation errors.

Agreed.

> The following semantic patch finds a leery xmalloc() caller in
> compat/mmap.c, though:
> 
> @@
> expression PTR, SIZE, SIZE2;
> @@
> (
>   PTR = xmalloc(SIZE);
> |
>   PTR = xcalloc(SIZE, SIZE2);
> |
>   PTR = xrealloc(SIZE);
> |
>   ALLOC_ARRAY(PTR, SIZE);
> |
>   CALLOC_ARRAY(PTR, SIZE);
> |
>   REALLOC_ARRAY(PTR, SIZE);
> )
>   if (
> - PTR == NULL
> + 0
>   ) {...}

IMHO that should not be using xmalloc() at all. It is a replacement for
system mmap, which can fail with ENOMEM, and we should be able to do the
same. Using xmalloc here is probably losing an opportunity to close
another pack window to free up memory for a new one.

I doubt it matters that much in practice (most systems are not even
compiling or using this code, and it would only matter in a tight memory
situation). But as a general rule, I think compat/ wrappers should
behave as much like (sensible) system equivalents as possible.

-Peff