Re: [PATCH] nilfs2: Fix reference count leak in nilfs_sysfs_create_snapshot_group()

Re: [PATCH] nilfs2: Fix reference count leak in nilfs_sysfs_create_snapshot_group()

[Markus Elfring]

> kobject_init_and_add() takes reference even when it fails.

It will be helpful to mention which object is referenced here, won't it?


> If this function returns an error, kobject_put() must be called to
> properly clean up the memory associated with the object.

I guess that an imperative wording is preferred also for this change description.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=b0c3ba31be3e45a130e13b278cf3b90f69bda6f6#n151

How do you think about to combine this update step together with
“nilfs2: Fix reference count leak in nilfs_sysfs_create_device_group”
into a small patch series?
https://lore.kernel.org/patchwork/patch/1248696/
https://lore.kernel.org/lkml/20200527200933.31135-1-wu000273@umn.edu/

Regards,
Markus

Re: nilfs2: Fix reference count leak in nilfs_sysfs_create_snapshot_group()

[Markus Elfring]

> I think there is only one object that can be modified in this function,

Such a view can be reasonable.


> so I didn't mention it.

I suggest to reconsider the conclusion.


>> I guess that an imperative wording is preferred also for this change description.
>
> This sentence is referenced from the code comment, so I haven't change it.
> https://elixir.bootlin.com/linux/v5.7-rc7/source/lib/kobject.c#L459

I find that that there are further possibilities to consider for improvements
around the presented commit message (even after the mentioned copy
from the function description of this programming interface).


>> How do you think about to combine this update step together with
>> “nilfs2: Fix reference count leak in nilfs_sysfs_create_device_group”
>> into a small patch series?
>
> I'd like to improve the similar issues after I reporting this bunch of bugs.

Did you find questionable implementation details with the help of an evolving
source code analysis tool?

Regards,
Markus