| Message ID | 1269973889-25260-1-git-send-email-ebiederm@xmission.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
Le mardi 30 mars 2010 à 11:31 -0700, Eric W. Biederman a écrit : > From: Eric W. Biederman <ebiederm@xmission.com> > > Add all of the necessary bioler plate to support > multiple superblocks in sysfs. > > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> > --- > fs/sysfs/mount.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > fs/sysfs/sysfs.h | 3 ++ > 2 files changed, 59 insertions(+), 2 deletions(-) > > diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c > index 0cb1088..6a433ac 100644 > --- a/fs/sysfs/mount.c > +++ b/fs/sysfs/mount.c > @@ -71,16 +71,70 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent) > return 0; > } > > +static int sysfs_test_super(struct super_block *sb, void *data) > +{ > + struct sysfs_super_info *sb_info = sysfs_info(sb); > + struct sysfs_super_info *info = data; > + int found = 1; > + return found; > +} > + > +static int sysfs_set_super(struct super_block *sb, void *data) > +{ > + int error; > + error = set_anon_super(sb, data); > + if (!error) > + sb->s_fs_info = data; > + return error; > +} > + > static int sysfs_get_sb(struct file_system_type *fs_type, > int flags, const char *dev_name, void *data, struct vfsmount *mnt) > { > - return get_sb_single(fs_type, flags, data, sysfs_fill_super, mnt); > + struct sysfs_super_info *info; > + struct super_block *sb; > + int error; > + > + error = -ENOMEM; > + info = kzalloc(sizeof(*info), GFP_KERNEL); > + if (!info) > + goto out; > + sb = sget(fs_type, sysfs_test_super, sysfs_set_super, info); > + if (IS_ERR(sb) || sb->s_fs_info != info) > + kfree(info); > + if (IS_ERR(sb)) { > + kfree(info); double kfree(info) ? > + error = PTR_ERR(sb); > + goto out; > + } > + -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Eric W. Biederman (ebiederm@xmission.com): > From: Eric W. Biederman <ebiederm@xmission.com> > > Add all of the necessary bioler plate to support > multiple superblocks in sysfs. > > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> Acked-by: Serge Hallyn <serue@us.ibm.com> > --- > fs/sysfs/mount.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > fs/sysfs/sysfs.h | 3 ++ > 2 files changed, 59 insertions(+), 2 deletions(-) > > diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c > index 0cb1088..6a433ac 100644 > --- a/fs/sysfs/mount.c > +++ b/fs/sysfs/mount.c > @@ -71,16 +71,70 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent) > return 0; > } > > +static int sysfs_test_super(struct super_block *sb, void *data) > +{ > + struct sysfs_super_info *sb_info = sysfs_info(sb); > + struct sysfs_super_info *info = data; > + int found = 1; > + return found; > +} > + > +static int sysfs_set_super(struct super_block *sb, void *data) > +{ > + int error; > + error = set_anon_super(sb, data); > + if (!error) > + sb->s_fs_info = data; > + return error; > +} > + > static int sysfs_get_sb(struct file_system_type *fs_type, > int flags, const char *dev_name, void *data, struct vfsmount *mnt) > { > - return get_sb_single(fs_type, flags, data, sysfs_fill_super, mnt); > + struct sysfs_super_info *info; > + struct super_block *sb; > + int error; > + > + error = -ENOMEM; > + info = kzalloc(sizeof(*info), GFP_KERNEL); > + if (!info) > + goto out; > + sb = sget(fs_type, sysfs_test_super, sysfs_set_super, info); > + if (IS_ERR(sb) || sb->s_fs_info != info) > + kfree(info); > + if (IS_ERR(sb)) { > + kfree(info); > + error = PTR_ERR(sb); > + goto out; > + } > + if (!sb->s_root) { > + sb->s_flags = flags; > + error = sysfs_fill_super(sb, data, flags & MS_SILENT ? 1 : 0); > + if (error) { > + deactivate_locked_super(sb); > + goto out; > + } > + sb->s_flags |= MS_ACTIVE; > + } > + > + simple_set_mnt(mnt, sb); > + error = 0; > +out: > + return error; > +} > + > +static void sysfs_kill_sb(struct super_block *sb) > +{ > + struct sysfs_super_info *info = sysfs_info(sb); > + > + kill_anon_super(sb); > + kfree(info); > } > > static struct file_system_type sysfs_fs_type = { > .name = "sysfs", > .get_sb = sysfs_get_sb, > - .kill_sb = kill_anon_super, > + .kill_sb = sysfs_kill_sb, > }; > > int __init sysfs_init(void) > diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h > 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)) > extern struct sysfs_dirent sysfs_root; > extern struct kmem_cache *sysfs_dir_cachep; > > -- > 1.6.5.2.143.g8cc62 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Serge E. Hallyn (serue@us.ibm.com): > Quoting Eric W. Biederman (ebiederm@xmission.com): > > From: Eric W. Biederman <ebiederm@xmission.com> > > > > Add all of the necessary bioler plate to support > > multiple superblocks in sysfs. > > > > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> > > Acked-by: Serge Hallyn <serue@us.ibm.com> (with the patch 7/6 of course :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Eric. On 03/31/2010 03:31 AM, Eric W. Biederman wrote: > From: Eric W. Biederman <ebiederm@xmission.com> > > Add all of the necessary bioler plate to support boiler :-) > +static int sysfs_test_super(struct super_block *sb, void *data) > +{ > + struct sysfs_super_info *sb_info = sysfs_info(sb); > + struct sysfs_super_info *info = data; > + int found = 1; > + return found; > +} Can you please make it return bool? > static int sysfs_get_sb(struct file_system_type *fs_type, > int flags, const char *dev_name, void *data, struct vfsmount *mnt) > { > - return get_sb_single(fs_type, flags, data, sysfs_fill_super, mnt); > + struct sysfs_super_info *info; > + struct super_block *sb; > + int error; > + > + error = -ENOMEM; > + info = kzalloc(sizeof(*info), GFP_KERNEL); > + if (!info) > + goto out; > + sb = sget(fs_type, sysfs_test_super, sysfs_set_super, info); > + if (IS_ERR(sb) || sb->s_fs_info != info) > + kfree(info); > + if (IS_ERR(sb)) { > + kfree(info); > + error = PTR_ERR(sb); > + goto out; > + } > + if (!sb->s_root) { > + sb->s_flags = flags; > + error = sysfs_fill_super(sb, data, flags & MS_SILENT ? 1 : 0); > + if (error) { > + deactivate_locked_super(sb); > + goto out; > + } > + sb->s_flags |= MS_ACTIVE; > + } > + > + simple_set_mnt(mnt, sb); > + error = 0; > +out: > + return error; > +} I haven't looked at later patches but I suppose this is gonna be filled with more meaningful stuff later. One (possibly silly) thing that stands out compared to get_sb_single() is missing remount handling. Is it intended? > 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? Thanks.
Tejun Heo <tj@kernel.org> writes: > Hello, Eric. > > On 03/31/2010 03:31 AM, Eric W. Biederman wrote: >> From: Eric W. Biederman <ebiederm@xmission.com> >> >> Add all of the necessary bioler plate to support > boiler :-) > >> +static int sysfs_test_super(struct super_block *sb, void *data) >> +{ >> + struct sysfs_super_info *sb_info = sysfs_info(sb); >> + struct sysfs_super_info *info = data; >> + int found = 1; >> + return found; >> +} > > Can you please make it return bool? Nope. That would mean I could not use it with sget. >> static int sysfs_get_sb(struct file_system_type *fs_type, >> int flags, const char *dev_name, void *data, struct vfsmount *mnt) >> { >> - return get_sb_single(fs_type, flags, data, sysfs_fill_super, mnt); >> + struct sysfs_super_info *info; >> + struct super_block *sb; >> + int error; >> + >> + error = -ENOMEM; >> + info = kzalloc(sizeof(*info), GFP_KERNEL); >> + if (!info) >> + goto out; >> + sb = sget(fs_type, sysfs_test_super, sysfs_set_super, info); >> + if (IS_ERR(sb) || sb->s_fs_info != info) >> + kfree(info); >> + if (IS_ERR(sb)) { >> + kfree(info); >> + error = PTR_ERR(sb); >> + goto out; >> + } >> + if (!sb->s_root) { >> + sb->s_flags = flags; >> + error = sysfs_fill_super(sb, data, flags & MS_SILENT ? 1 : 0); >> + if (error) { >> + deactivate_locked_super(sb); >> + goto out; >> + } >> + sb->s_flags |= MS_ACTIVE; >> + } >> + >> + simple_set_mnt(mnt, sb); >> + error = 0; >> +out: >> + return error; >> +} > > I haven't looked at later patches but I suppose this is gonna be > filled with more meaningful stuff later. Yes it will. > One (possibly silly) thing > that stands out compared to get_sb_single() is missing remount > handling. Is it intended? There is nothing for a remount to do so I ignore it. The only thing that would possibly be meaningful is a read-only mount, and nothing I know of sysfs suggests read-only mounts of sysfs work, or make any sense. >> 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. Eric -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -serge -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
"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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Eric. On 03/31/2010 02:51 PM, Eric W. Biederman wrote: >> I haven't looked at later patches but I suppose this is gonna be >> filled with more meaningful stuff later. > > Yes it will. > >> One (possibly silly) thing >> that stands out compared to get_sb_single() is missing remount >> handling. Is it intended? > > There is nothing for a remount to do so I ignore it. The only > thing that would possibly be meaningful is a read-only mount, > and nothing I know of sysfs suggests read-only mounts of sysfs > work, or make any sense. I see. Wouldn't it be better to make that design choice evident by stating the choice in the comment or at least in the patch description? As it currently stands, you're burying a clear functional change in a seemingly innocent patch which contains zero line of comment and two lines of description. The same pattern holds for this whole patchset. Where are the comments and descriptions about the design and implementation? :-( Thanks.
diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c index 0cb1088..6a433ac 100644 --- a/fs/sysfs/mount.c +++ b/fs/sysfs/mount.c @@ -71,16 +71,70 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent) return 0; } +static int sysfs_test_super(struct super_block *sb, void *data) +{ + struct sysfs_super_info *sb_info = sysfs_info(sb); + struct sysfs_super_info *info = data; + int found = 1; + return found; +} + +static int sysfs_set_super(struct super_block *sb, void *data) +{ + int error; + error = set_anon_super(sb, data); + if (!error) + sb->s_fs_info = data; + return error; +} + static int sysfs_get_sb(struct file_system_type *fs_type, int flags, const char *dev_name, void *data, struct vfsmount *mnt) { - return get_sb_single(fs_type, flags, data, sysfs_fill_super, mnt); + struct sysfs_super_info *info; + struct super_block *sb; + int error; + + error = -ENOMEM; + info = kzalloc(sizeof(*info), GFP_KERNEL); + if (!info) + goto out; + sb = sget(fs_type, sysfs_test_super, sysfs_set_super, info); + if (IS_ERR(sb) || sb->s_fs_info != info) + kfree(info); + if (IS_ERR(sb)) { + kfree(info); + error = PTR_ERR(sb); + goto out; + } + if (!sb->s_root) { + sb->s_flags = flags; + error = sysfs_fill_super(sb, data, flags & MS_SILENT ? 1 : 0); + if (error) { + deactivate_locked_super(sb); + goto out; + } + sb->s_flags |= MS_ACTIVE; + } + + simple_set_mnt(mnt, sb); + error = 0; +out: + return error; +} + +static void sysfs_kill_sb(struct super_block *sb) +{ + struct sysfs_super_info *info = sysfs_info(sb); + + kill_anon_super(sb); + kfree(info); } static struct file_system_type sysfs_fs_type = { .name = "sysfs", .get_sb = sysfs_get_sb, - .kill_sb = kill_anon_super, + .kill_sb = sysfs_kill_sb, }; int __init sysfs_init(void) diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h 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)) extern struct sysfs_dirent sysfs_root; extern struct kmem_cache *sysfs_dir_cachep;