[pulseaudio-discuss] asserts active by default

[Lennart Poettering]

On Tue, 02.09.08 14:58, Jan-Benedict Glaw (jbglaw at lug-owl.de) wrote:

> > > I've seen in other projects a "--disable-assert" argument to the
> > > ./configure script. Would that (or --enable-assert) be an option to
> > > consider?
> > 
> > The asserts indicate when an invalid use of the API is encountered (or a 
> > bug of course!). When an assert happens it indicates an error condition 
> > and in pulse, essentially protect against buffer overflows and other 
> > such conditions.
> 
> You're mixing up two things here.
> 
> Invalid API usage should be signaled with a NULL return, or -1, or
> whatever makes sense. The same is true for recoverable internal
> errors, which additionally could be asserted on in case of NDEBUG not
> being set.
> 
> But pulse behaves different here. A lot of circumstances where
> everybody and her turtle would just return an error, pulse will hit an
> assertion.

"Just return an error"?

In the PA core I use asserts wherever a programming error is
obvious. Which is the right thing to do. (Note that I am am talking of
the core here, not the client libs!)

It is naive to believe that "just returning an error" would be a good
option in all cases including programming errors in complex programs
like in PA. Error paths can never be tested comprehensively, which
means they are usually much more buggy than the paths where everything
is behaving correctly. Which means: if it is you who fucked up, then
admit it, don't try to fuck it up even further by trying to be smart
and come up with "Plan Bs" or something that try to fix your
programming errors without you even knowning them. The crux of
handling programming errors is that you can never know what their nature
actually is. Because if you new, you'd have fixed them anyway, right?

Don't try to hide your programming errors. If you try, you lost already.

So, if you believe that everyone and his turtle is doing right by
"just returning an error" everywhere, then that 'everyone' must have
psychic powers -- or simply no clue. ;-)

Catching common-case runtime errors is hard enough. Spend your time on
coming up with error paths for them. Don't try to come up with errors
paths for your own programming errors! Spend the time of fixing them
instead.

> > If the asserts were disabled, then a significant rework in terms of 
> > error handling would need to be performed. If a catchable error occurs 
> > in pulse, an assert is not used.
> 
> Right, because error handling and assertions were mixed up. Error
> handling is for errors that could expectedly happen (eg. malloc()
> returns a NULL pointer, user requests an operation that's invalid in
> this context, ...)

malloc() returning NULL is not an error that can realisticly
happen. If malloc() returns NULL you are fucked anyway.

Since modern operating systems work the way they work, the result of
OOM is not that malloc() returns NULL, but that you are being killed
by the OOM killer. The reason for that is that memory pages are only
allocated when they are used -- not already when you call malloc(). 

On modern operating systems malloc() doesn't reserve memory, it only
reserves address space. If malloc() returns NULL then you managed to
reserve up to the limit of 2GB of memory (on x86). If you did that
then you deserve being aborted.

Due to this all reasonable software (unless it is very low-level or
kernel stuff) doesn't try to handle OOM. It just aborts. All GLib/Gtk
programs are one example. PA is another one.

Appropriate error checking is for stuff like permission errors on
open(), disk full, and so on. Testing the result of malloc is not a
good example.

> Assertions should be put on conditions that a user should *never* be
> able to provoke in *any* circumstance, like if we *know* we
> initialized an internal variable to 1 upon open, we *know* the user
> never closed a handle (where it would be set to 0) and we find out
> that this given variable is 0, while the handle was never being
> closed.

Exactly. And that's how PA uses asserts in the core. Since the core is
self-contained I know that if an invalid parameter appears somewhere I
am the one to blame, and I did a programming error of some kind.

> > I'm not really sure of the value in disabling asserts. If they are being 
> > hit, then this just means there are bugs to be fixed!
> 
> It'll provoke undefined behaviour due to now missing error handling.

No. That's nonsense. See my other mails.

> > But perhaps I don't get something here, so will wait for a more 
> > authoritative answer from Lennart on this :)
> 
> I'm quite a happy pulse user, but I found out about this long ago.
> ISTR that I even wrote an email at that time, but I'm not sure. I also
> seem to remember that Lennart considered audio output unimportant
> enough to simply let it run into an assertion instead of clobbering
> the code with overly complete error handling.

Yes. Having error paths for programming errors is making code
complex to read, for no benefit.

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4