Hi Ilpo,
On 10/11/23 8:14 pm, Ilpo Järvinen wrote:
On Fri, 10 Nov 2023, Harshit Mogalapalli wrote:
Thanks for the review.
This changelog needs to be rewritten, it contains multiple errors. I
suppose even this patch could be split into two but I'll not be too picky
here if you insist on fixing them in the same patch.
Any thoughts on how to split this into two patches ?
I thought of fixing memory leak in separate patch, but that would add
more code which should be removed when we move kobject_put() to the end.
We have two issues:
1. Memory leak of 'attr_name_kobj' in the error handling path.
True, but not specific enough to be useful.
Should I mention something like:
'attr_name_kobj' is allocated using kzalloc, but on all the error paths
we don't free it, hence we have a memory leak.
2. When kobject_init_and_add() fails on every subsequent error path call
kobject_put() to cleanup.
This makes no sense. The only case when there old code had no issue is
"when kobject_init_and_add() fails" but now your wording claims it to be
source of problem. Please rephrase this.
Does this look better:
kobject_put() must be called to cleanup memory associated with the
object if kobject_init_and_add() returns an error , before this patch
only the error path which is immediately next to kobject_init_and_add()
has a kobject_put() not any other error paths after it. Fix this by
moving the kobject_put() into a goto label "err_other_attr_init:" and
use that for error paths after kobject_init_and_add().
Both of these issues will be fixed when we add kobject_put() in the goto
label, as kfree() is already part of kobject_put().
No, you're fixing a problem in the patch which is not covered by moving
kobject_put()!
Sure, I will try to rephrase it to:
1. Add a new label "unlock_drv_mutex"
2. Add a kfree() in the default statement of switch before going to
"unlock_drv_mutex" to free up the memory allocated with kzalloc.
2. Move kobject_put() to goto "err_other_attr_init:" and use this goto
label in every error path after kobject_init_and_add().
Please let me know if you have any comments.
Thank you very much.
Regards,
Harshit
