[systemd-devel] [PATCH] libsystemd-bus: Clean up code

[Lennart Poettering]

On Tue, 10.12.13 18:33, Lennart Poettering (lennart at poettering.net) wrote:

> On Tue, 10.12.13 05:24, Zbigniew Jędrzejewski-Szmek (zbyszek at in.waw.pl) wrote:
> 
> > On Mon, Dec 09, 2013 at 09:22:43PM +0100, Thomas H.P. Andersen wrote:
> > > If I understood correctly then the assert_return was meant to be used
> > > only on the public library functions. I can't seem to find the
> > > reference to it so maybe I am wrong though.
> >
> > There's no strong reason to limit usage to public functions. It's just
> > a simple macro really.
> 
> It's actually more complicated than that. The macro uses _unlikely_(),
> which only should be used when we know that something is really really
> unlikely. For assert()-style checks we know that thigns are unlikely,
> since they are used to detect programming errors only, not runtime
> errors.
> 
> hence: please use assert_return() only for checks where normally an
> assert() would be appropriate too, but where this would be too
> unfriendly because it's in user-facing API. It's about being forgiving
> to programming errors by users of our code, but not to ourselves.
> 
> To underline this I have now updated assert_return() to also log the
> failing condition as LOG_DEBUG, since that sounds like a good idea so
> that programming errors can be easily tracked.
> 
> So again: please do not replace if checks blindly by
> assert_return(). Use assert_return() only to detect programming errors
> in user code, which means only really to check parameters of public API
> calls. Use assert() in internal code for the same purpose.
> 
> I also updated CODING_STYLE a bit to explain this.

I have reverted this commit btw. This actually did blow up for example
in bus_kernel_pop_memfd() which we call all the time on non-kdbus
systems and which is supposed to fail there. Since assert_return()
includes _likely_() using it here will hence slow things down quite
a bit. Also, with my recent logging changes for assert_return() you
actually get a lot of debug spew up this too, so it becomes
extra-expensive.

THe other changes in the same patch appeared to be in the same category:
where it wasn't obvious that the checks would only detect programming
errors and not happen in normal control flow. I have thus reverted the
entire patch.

Lennart

-- 
Lennart Poettering, Red Hat