Hi On Wed, May 13, 2020 at 7:54 PM Andrew Gregory <andrew.gregor...@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 *does* evaluate 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 functionality. > 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 https://github.com/anatol/pacman/blob/b96e0df4dceaa2677baa1a3563211950708d3e63/src/pacman/callback.c#L850 > 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 loosing functionality. > The > 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?
