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. 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. At that point bar and index are indeterminate and the best we can hope for is a quick segfault. The callback api should be fixed so that callbacks can indicate an error that should cancel the download. 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.