Messages in this thread | | | Subject | Re: [PATCH 1/6] sysfs: Basic support for multiple super blocks | From | (Eric W. Biederman) | Date | Wed, 31 Mar 2010 07:02:39 -0700 |
| |
"Serge E. Hallyn" <serue@us.ibm.com> writes:
> Quoting Eric W. Biederman (ebiederm@xmission.com): >> Tejun Heo <tj@kernel.org> writes: >> >> index 30f5a44..030a39d 100644 >> >> --- a/fs/sysfs/sysfs.h >> >> +++ b/fs/sysfs/sysfs.h >> >> @@ -114,6 +114,9 @@ struct sysfs_addrm_cxt { >> >> /* >> >> * mount.c >> >> */ >> >> +struct sysfs_super_info { >> >> +}; >> >> +#define sysfs_info(SB) ((struct sysfs_super_info *)(SB->s_fs_info)) >> > >> > Another nit picking. It would be better to wrap SB in the macro >> > definition. Also, wouldn't an inline function be better? >> >> Good spotting. That doesn't bite today but it will certainly bite >> someday if it isn't fixed. >> >> I wonder how that has slipped through the review all of this time. > > (let me demonstrate how: ) > > WTH are you talking about? Unless you mean doing (SB) inside > the definition? > > I actually was going to suggest dropping the #define as it obscures > the code, but I figured it would get more complicated later.
I believe the discuss change was to make the define: #define sysfs_info(SB) ((struct sysfs_super_info *)((SB)->s_fs_info))
As for dropping the define and using s_fs_info raw. I rather like a light weight type safe wrapper. Maybe I just think s_fs_info is an ugly name.
In practice I never call sysfs_info() with any expression that has a side effect, so it doesn't matter.
Eric
| |