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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx> 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@xxxxxxxxx>
> > > 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@xxxxxxxxx>
> > > 
> > > Reviewed-by: NeilBrown <neilb@xxxxxxxx>
> > > 
> > > 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?

--b.

> 
> 
> Thanks for accepting the other patches!
> 
> NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux