On 3/29/23 08:18, Markus Elfring wrote: >> It is valid to pass a NULL pointer to kfree(), it checks for that. >> >>> I imagine that such code should be reconsidered once more and improved accordingly. >> >> The only potential downside in the scenario is checking storage == NULL twice. > > Will programming concerns evolve any further? It's very unlikely that the behavior of kfree(NULL) will change. >> But this is an error handling path, not a fast path. > > I hope that also exception handling code can become efficient in more use cases. Nobody will notice the efficiency, and it's not the only goal, code readability is also important. >> On the other hand, the code may look like this: >> >> storage = call(...); >> if (!storage) >> goto target; >> >> if (some_other_condition) >> goto target; >> >> target: >> kfree(storage) >> >> >> For some_other_condition, we want the kfree(). > > Allocated resources should be properly released. Sure. They are properly released in the above example. >> If you changed the code, to remove the extra NULL pointer check, you would have: >> >> storage = call(...); >> if (!storage) >> goto target1; >> >> if (some_other_condition) >> goto target2; >> >> target2: >> kfree(storage) > > A label like “free_this_resource” can be nicer, can't it? Sure, name was not the point. Just that there are now 2 labels. >> target1: >> >> The extra goto label and larger code is not worth saving the NULL pointer check, >> because this is error handling path. > > Some code reviewers and programmers represent other development views. That's the universal truth :) > >>> How do you think about to achieve any adjustments in this design direction? >> >> Only in cases where it would not make the code more complex, i.e. if there >> are no "some_other_condition" that jumps to the same target after allocation >> to storage is successful. > > Do you find any implementation details worth for another look also according to > a special information source? > https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources#MEM12C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution%28POSIX,GotoChain%29 > > > Will circumstances evolve in ways so that you would adjust a desire to stick > to only a single jump label? Depends on specific code cleanup. But generally, not if the change is only to add an extra label to skip over kfree() - I will unlikely consider such patches as useful. > Regards, > Markus
