Re: [PATCH 2/2] mdadm: add map_num_s()

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Mon, 4 Apr 2022 21:32:09 -0400
Jes Sorensen <jes@trained-monkey.org> 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