RE: [PATCH 14/15] kernel: convert futex_pi_state.refcount from atomic_t to refcount_t
On Mon, 17 Jul 2017, Elena Reshetova wrote:
> Subject: kernel: convert futex_pi_state.refcount from atomic_t to refcount_t
Several people including myself told you already, that subjects consist of
SUBSYSTEMPREFIX: Concise description
It's easy enough to figure the prefix out by looking at the output of 'git
log path/of/changed/file'.
Ok, I will try this from now on. I didn't think of it, but was trying to figure it
based on general location and meaning (obviously wrong).
Concise descriptions are not lengthy sentences with implementation
details. They should merily describe the problem/concept of change. The
details go into the changelog. IOW, something like:
"PROPERPREFIX: Convert to refcount API"
would be sufficient.
OK, will fix.
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
Copying the same sentence over and over avoids thinking about a proper
changelog, right? You fail to explain, how you come to the conclusion that
futex_pi_state.refcount is a pure reference counter (aside of the name) and
therefor can be safely converted to refcount_t.
OK, this is not very useful for many cases. Yes, I am using automated log on
these patches, because I used to have 240 of them and writing manual logs for them
would be fun. Moreover, in many cases, writing manual logs don't bring any value
since
I would have to repeat the same things all over again: xyz conversions was found by using
*.cocci
pattern first, then looked at manually and it looked like a standard reference counter
that
frees the things after calling refcount_dec_and_test() (or its variation with lock which
is rare).
Other things also looked correct, like I didn't see increments from zero, counter
starts at 1 etc.
I would really have to repeat the same thing in each changelog. Does it really bring
value?
Best Regards,
Elena.
Other than that, the patch itself is fine.
Thanks,
tglx