On Mon, 4 Apr 2022 21:32:09 -0400 Jes Sorensen <jes@xxxxxxxxxxxxxxxxxx> wrote: > On 1/20/22 07:18, Mariusz Tkaczyk wrote: > > map_num() returns NULL if key is not defined. This patch adds > > alternative, non NULL version for cases where NULL is not expected. > > > > There are many printf() calls where map_num() is called on variable > > without NULL verification. It works, even if NULL is passed because > > gcc is able to ignore NULL argument quietly but the behavior is > > undefined. For safety reasons such usages will use map_num_s() now. > > It is a potential point of regression. > > Hi Mariusz, > > I'll be honest with you, I don't like assert(), I consider it a lame > excuse for proper error handling. That said, not blaming you as this > is old code and it would take a lot of cleaning up, so this is better > than nothing. And that is true, assert() is not for errors handling. Is was made for verifying function/application flows. Like here, I made not null function and I guarantee that won't be returned. If it comes, then it is a developer mistake and assert() should discover that during testing. Please see man: https://man7.org/linux/man-pages/man3/assert.3.html If the macro NDEBUG is defined at the moment <assert.h> was last included, the macro assert() generates no code, and hence does nothing at all. It is not recommended to define NDEBUG if using assert() to detect error conditions since the software may behave non-deterministically. For production, the macro should be set and it generates no code. So the behavior is configurable and OSV should add this flag to their mdadm builds. In this case, we will end with previous implementation but no additional error handling is required to satisfy our and external requirements (like static code analysis). It is also useful to validate function parameters which must be set, to not insert dead conditions. Ideally, we should execute test with asserts enabled to verify use cases. Thanks, Mariusz
