Re: [PATCH 1/9 v8] fs_pin: Initialize value for fs_pin explicitly

[Kinglong Mee <kinglongmee@gmail.com>]

On 7/30/2015 08:36, J. Bruce Fields wrote:
> On Thu, Jul 30, 2015 at 07:48:24AM +1000, NeilBrown wrote:
>> On Wed, 29 Jul 2015 15:41:55 -0400 "J. Bruce Fields"
>> <bfields@fieldses.org> wrote:
>>
>>> On Wed, Jul 29, 2015 at 10:25:19AM +1000, NeilBrown wrote:
>>>> On Mon, 27 Jul 2015 11:06:53 +0800 Kinglong Mee <kinglongmee@gmail.com>
>>>> wrote:
>>>>
>>>>> Without initialized, done in fs_pin at stack space may
>>>>> contains strange value.
>>>>>
>>>>> v8, same as v3
>>>>> Adds macro for header file
>>>>>
>>>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>>>
>>>> Reviewed-by: NeilBrown <neilb@suse.com>
>>>>
>>>> It would be really good if some of these early patches could be applied
>>>> to the relevant trees so they appear in -next and we only need to keep
>>>> reviewing the more interesting code at the end.
>>>
>>> This patch seems a little bikeshed-y.  I'd rather just drop it or save
>>> it for some other day.  It's not necessary to the series.
>>
>> ???
>>
>> I accept that:
>>
>>
>>>>> @@ -1,3 +1,6 @@
>>>>> +#ifndef _LINUX_FS_PIN_H
>>>>> +#define _LINUX_FS_PIN_H
>>>>> +
>>>>>  #include <linux/wait.h>
>>
>> could be a little bike-shed-y, not that I've seen much bike shedding
>> going on.
>>
>> However:
>>>>>  
>>>>>  struct fs_pin {
>>>>> @@ -16,9 +19,12 @@ static inline void init_fs_pin(struct fs_pin *p, void (*kill)(struct fs_pin *))
>>>>>  	INIT_HLIST_NODE(&p->s_list);
>>>>>  	INIT_HLIST_NODE(&p->m_list);
>>>>>  	p->kill = kill;
>>>>> +	p->done = 0;
>>>>>  }
>>>>>  
>>
>> is quite important.
>> Without that assignment we would probably need to rename the function to
>>    init_most_of_fs_pin
>> or
>>    init_fs_pin_if_already_zeroed
>> or maybe just
>>    __init_fs_pin
>> with the time honoured interpretation that it sort-of does what the
>> name says, but maybe not exactly how you think and please use with care.
>>
>> Then in nfsd code we would need to add the assignment ourselves, or use
>> kzalloc where it would otherwise be completely unnecessary.
> 
> Right, the existing users do something like kzalloc, last I checked.
> Maybe it'd be an improvement not to.
> 
> I don't really care, but since Al's a bit overloaded, and you don't want
> to see this reposted, and it's not really essential to the series, why
> not drop it for now?

What's your opinion about this patch, Al?
Drop? needs update?

thanks,
Kinglong Mee