On Thu, Nov 03, 2016 at 06:27:21PM +0100, Markus Teich wrote:
> Heyho,
>
Hiya!
> Davids patch for sent reminded me of an open issue. I proposed the following
> change to libsls ecalloc() a few months ago, but did not get feedback.
>
> void *
> ecalloc(size_t nmemb, size_t size)
> {
> void *p;
>
> if (!(p = calloc(nmemb, size)))
> - perror(NULL);
> + die("calloc:");
> return p;
> }
>
> The three projects using libsl I know of are dwm, dmenu and sent.
>
> ecalloc() is used by drw.c when allocating the main struct Drw, fontsets, colors
> and cursors. Currently only the colors and cursors case are non-fatal, because
> the return code is checked. When the ecalloc fails for the Drw or fontset
> allocations, the application immediately references those and therefore crashes
> with a segfault.
>
drw.c is not a library in my eyes so fatal errors are ok there I think.
> In dwm there are three usages (new monitor, new client, XineramaScreenInfo) as
> well, all of them with immediate references and therefore segfaults.
>
> In dmenu there are no uses of ecalloc() apart from the ones implied by drw.c
> usage and in sent there are currently no calls to ecalloc() as well, however I'd
> like to introduce it there with the die() behavior.
>
It's definitely a bug.
> In sbase/ubase all the allocation helpers immediately quit the programm on
> failure.
>
>
> Now to the big question: Should we change ecalloc to quit the programm
> immediately via die() on an allocation failure, or should we leave it as is and
> let applications check if they want to?
>
Definitely change it to sbase/ubase behaviour.
> There is a third option: Introducing wecalloc() which only prints an error
> message, but does not quit the programm and changing ecalloc() to the die()
> semantic.
>
Strongly disagree with wecalloc(). I don't even mind removing ecalloc() if there
are only a few allocation cases in the program.
> I can at least imagine cases where quitting on allocation failure is not good.
> For example dwm is running, a new client starting up, but there is no memory
> left for the client struct. In this case dwm should just print an error, but not
> quit to give the user the chance to fix the problem without losing all their
> work in the other clients (which would die as well, if dwm dies and the X-Server
> quits).
>
I agree it should either die completely or give a very clear error message.
> My proposal would be to change ecalloc to use die() and in the rare cases where
> the allocation error should be handled gracefully just don't use ecalloc, but
> calloc directly.
>
> Btw: The drw unification patches are still not merged to libsl, only to dwm,
> dmenu and sent.
>
Do you want to (re)send the patch or should I just change it?
--
Kind regards,
Hiltjo
Received on Fri Nov 04 2016 - 17:18:34 CET