On 3/21/24 04:29, Jakub Kicinski wrote:
On Wed, 20 Mar 2024 08:01:49 +0300 Dan Carpenter wrote:
This is just trading one kind of bug for another, and the __free()
magic is at a cost of readability.
Apologies for not catching it during review.
It's good that we have started small, with just a few functions.
I think we should ban the use of __free() in all of networking,
until / unless it cleanly handles the NULL init case.
Current API is indeed asking for bugs, especially when combined with RCT
and early error checking rules. Perhaps that's why there is double
underscore prefix ;)
Simplest solution would be to add a macro wrapper, especially that there
are only a few deallocation methods.
in cleanup.h:
+#define auto_kfree __free(kfree) = NULL
and similar macros for auto vfree(), etc.
then in the drivers:
-struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL,
*othercaps __free(kfree) = NULL;
+struct ice_aqc_get_phy_caps_data *pcaps auto_kfree,
*othercaps auto_kfree;
With that only developers introducing new allocators/wrappers would be
using bare __free(), the rest of us will be free to focus on other
things.
One could argue (+CC David Laight) that additional zero-init would not
be wiped out by compiler, but that is a price I would happily pay in
almost all cases.
I have no idea if someone already proposed exactly that, as this is
almost obvious solution.
Free handles the NULL init case, it doesn't handle the uninitialized
case. I had previously argued that checkpatch should complain about
every __free() pointer if the declaration doesn't have an assignment.
The = NULL assignment is unnecessary if the pointer is assigned to
something else before the first return, so this might cause "unused
assignment" warnings? I don't know if there are any tools which
complain about that in that situation. I think probably we should just
make that an exception and do the checkpatch thing because it's such a
simple rule to implement.
What I was trying to say is that the __free() thing is supposed to
prevent bugs, and it's not. Even if it was easy to write the matcher
rule, if __free() needs a rule to double check its use - it's failing
at making it easier to write correct code.
In any case. This is a patch for Intel wired, I'll let Intel folks
decide.
