On Mon, Oct 14, 2024 at 10:15:25AM +0200, Javier Carrasco wrote:
On 14/10/2024 10:12, Dan Carpenter wrote:
On Mon, Oct 14, 2024 at 09:59:49AM +0200, Javier Carrasco wrote:
This approach is great as long as the maintainer accepts mid-scope variable declaration and the goto instructions get refactored, as stated in cleanup.h.
The first point is not being that problematic so far, but the second one is trickier, and we all have to take special care to avoid such issues, even if they don't look dangerous in the current code, because adding a goto where there cleanup attribute is already used can be overlooked as well.
To be honest, I don't really understand this paragraph. I think maybe you're talking about if we declare the variable at the top and forget to initialize it to NULL? It leads to an uninitialized variable if we exit the function before it is initialized.
No, I am talking about declaring the variable mid-scope, and later on adding a goto before that declaration in a different patch, let's say far above the variable declaration. As soon as a goto is added, care must be taken to make sure that we don't have variables with the cleanup attribute in the scope. Just something to take into account.
Huh. That's an interesting point. If you have:
if (ret) goto done;
struct device_node *fw_node __free(device_node) = something;
Then fw_node isn't initialized when we get to done. However, in my simple test this triggered a build failure with Clang so I believe we would catch this sort of bug pretty quickly.
regards, dan carpenter