Dear Tom,
In message 20210819130806.GW858@bill-the-cat you wrote:
So we have now a policy to wave through code, and ask others to clean it up later? That's ... sad.
No, we continue to have the policy of expecting reviewers to follow the whole discussion and relevant subsystems.
Once upon a time there has also been a policy that if a function might return error codes, these need to be checked and handled.
Changing _every_ caller of dev_get_priv to check for NULL and then, what? is clearly not the right answer.
Then what is the right answer in your opinion?
I mean, look at the implementation of dev_get_priv():
628 void *dev_get_priv(const struct udevice *dev) 629 { 630 if (!dev) { 631 dm_warn("%s: null device\n", __func__); 632 return NULL; 633 } 634 635 return dm_priv_to_rw(dev->priv_); 636 }
If there is guaranteed no way that dev_get_priv() can return a NULL pointer, that means that it must be guaranteed that the "dev" argument can never be a NULL pointer, either. So why do we check it at all?
The same applies for all functions in "drivers/core/device.c" - they all check for valid input parameters, like any code should do.
If you think you see a problem here please go audit the DM code itself more and propose some changes.
I can see that the DM code itself implements proper error checking and reporting; it's the callers where negligent code prevails.
If you are consequent, you must decide what you want:
- Either we want robust and reliable code - then we have to handle the error codes which functions like dev_get_priv() etc. return.
- Or you don't care about software quality, then we can omit such handling, but then it would also be consequent to remove all the error checking from "drivers/core/device.c" etc. - hey, that would even save a few hundred bytes of code size.
Sugarcoating code which fails to handle error codes because "these errors can never happen" does not seem to be a clever approach to software engineering to me.
I stop here. You know my opinion. You are the maintainer.
Wolfgang Denk