Hi,
On Thu, 19 Aug 2021 at 08:16, Wolfgang Denk wd@denx.de wrote:
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.
Note that we should not check these calls, as the priv data is allocated by driver model and a device cannot be probed until it gets past this step.
I know that sometimes people add this check, but whenever I see it I ask them to remove it. It is pointless and confusing, since it suggests that driver model may not have allocated it yet. This sort of confusion can really make things hard to understand for new people.
Then what is the right answer in your opinion?
If a device is probed, you can rely on the priv data being set up. The only way to access a probed device is via the device-internal.h or uclass-internal.h APIs. Be careful with those if you must use them.
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.
I think did a series on this many years ago - a way to turn on checks for this and that the right struct is obtained when you call dev_get_priv(), etc. We could certainly add this with a CONFIG option for debugging purposes. The runtime cost is actually not terrible but it does require collusion with malloc() to be efficient.
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.
There is a wider issue here about arrg checking. We sometimes use assert but at present that only appears in the code if DEBUG is enabled. Also it just halts.
OTOH if we add arg checking everywhere it cluttles the code and can substantially increase the size (I know of a project where it doubled the size). I like to distinguish between:
- programming errors - security errors where user input is insufficiently checked
IMO the former should not be present if you have sufficient tests and trying to catch them in the field at runtime is not very kind to your users.
Regards, Simon