[pacman-dev] [PATCH] Avoid depending on side effects in assert(...) expressions
anatol.pomozov at gmail.com
Fri May 15 06:30:02 UTC 2020
On Wed, May 13, 2020 at 7:54 PM Andrew Gregory
<andrew.gregory.8 at gmail.com> wrote:
> On 05/13/20 at 12:08pm, Anatol Pomozov wrote:
> > > ---
> > > It's perhaps worth mentioning that nowhere else in the ALPM codebase
> > > do we use assert().
> > I quite like the idea of defensive programming. This is something that
> > I learnt the hard way when I was working with chips firmware.
> > So I often add additional checks across the codebase and it saves me
> > time during active phase of development/refactoring.
> > A bit of context for this assert(). Both "progress" and "complete"
> > events should always have a corresponding "init" event where
> > progressbar structure is initialized.
> > If callback.c received a "progress" event for a non-existent
> > progressbar then it is a programming error.
> The problem is not defensive programming, it's that assert gets
> compiled out under -DNDEBUG, so all your defenses disappear.
True, assert() does *not* evaluate parameters if -DNDEBUG is used.
I was confused by asserts at other systems like Linux's BUG_ON() that
parameters even if BUG() support is disabled.
Dave's patch makes what I intended to do - evaluate the statement and
then call assert()
to check the results. It is OK to disable asserts and it should not
affect the application
> You say
> that the only way there would not be a corresponding progress bar is
> if the progress event is called without an init event; that's not
> true. If there's a memory allocation failure in the init event there
> won't be a progress object.
If there is a malloc failure in the init callback then application
will crash with SIGSEGV while
trying to access the NULL pointer
> At that point bar and index are
> indeterminate and the best we can hope for is a quick segfault.
an application crash is pretty much what assert() does here. It
basically checks the pointer before we start using it.
In some sense assert() is a form of a self-documenting check
"this situation should never happen, but if it does then it is a bug
in the app logic. so let's crash the app to bring
maximum attention to it".
These asserts() are extra-checks but they are not required. It can be
disabled or even removed without
> callback api should be fixed so that callbacks can indicate an error
> that should cancel the download.
It sounds like a good idea to return an error code from the callback.
In this case we can
handle malloc() failure in init() without crashing the application.
But I do not think it should block the multi-download testing. It
would be great to get people testing the new functionality
to discover issues as soon as possible.
> I'd also say we should be passing the full list of download items to
> the callback so that it's possible to write at least a very simple
> multi-download callback without having to maintain a bunch of
> complicated state information.
I am not sure I fully understand this idea. Could you please expand more on it?
More information about the pacman-dev