Message ID | 20171127231801.27652-2-sirmy15@gmail.com |
---|---|
State | Accepted, archived |
Headers | show |
Series | Improve kobject handling in fs/ext4 | expand |
On Nov 27, 2017, at 4:17 PM, Riccardo Schirone <sirmy15@gmail.com> wrote: > > Even when kobject_init_and_add/kset_register fail, the kobject has been > already initialized and the refcount set to 1. Thus it is necessary to > release the kobject/kset, to avoid the memory associated with it hanging > around forever. This seems like a bad programming model. It doesn't make sense if the "init" or "register" function returns an error that you would still have to call "put" or "unregister" on the object. Why not just handle the cleanup at the end of "kobject_init_and_add()" or "kobject_register()" if there is an error instead of putting the burden on every caller? If open() returns an error, I don't need to call close(), and if malloc() fails I don't need to call free(), etc. Cheers, Andreas > Signed-off-by: Riccardo Schirone <sirmy15@gmail.com> > --- > fs/ext4/sysfs.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c > index 48c7a7d55ed3..b096e157934f 100644 > --- a/fs/ext4/sysfs.c > +++ b/fs/ext4/sysfs.c > @@ -395,8 +395,11 @@ int ext4_register_sysfs(struct super_block *sb) > init_completion(&sbi->s_kobj_unregister); > err = kobject_init_and_add(&sbi->s_kobj, &ext4_sb_ktype, NULL, > "%s", sb->s_id); > - if (err) > + if (err) { > + kobject_put(&sbi->s_kobj); > + wait_for_completion(&sbi->s_kobj_unregister); > return err; > + } > > if (ext4_proc_root) > sbi->s_proc = proc_mkdir(sb->s_id, ext4_proc_root); > @@ -429,15 +432,19 @@ int __init ext4_init_sysfs(void) > kobject_set_name(&ext4_kset.kobj, "ext4"); > ext4_kset.kobj.parent = fs_kobj; > ret = kset_register(&ext4_kset); > - if (ret) > + if (ret) { > + kset_unregister(&ext4_kset); > return ret; > + } > > ret = kobject_init_and_add(&ext4_feat, &ext4_feat_ktype, > NULL, "features"); > - if (ret) > + if (ret) { > + kobject_put(&ext4_feat); > kset_unregister(&ext4_kset); > - else > + } else { > ext4_proc_root = proc_mkdir(proc_dirname, NULL); > + } > return ret; > } > > -- > 2.14.3 > Cheers, Andreas
On 11/27, Andreas Dilger wrote: > On Nov 27, 2017, at 4:17 PM, Riccardo Schirone <sirmy15@gmail.com> wrote: > > > > Even when kobject_init_and_add/kset_register fail, the kobject has been > > already initialized and the refcount set to 1. Thus it is necessary to > > release the kobject/kset, to avoid the memory associated with it hanging > > around forever. > > This seems like a bad programming model. It doesn't make sense if the > "init" or "register" function returns an error that you would still have > to call "put" or "unregister" on the object. Why not just handle the > cleanup at the end of "kobject_init_and_add()" or "kobject_register()" > if there is an error instead of putting the burden on every caller? > > If open() returns an error, I don't need to call close(), and if malloc() > fails I don't need to call free(), etc. > > Cheers, Andreas > I agree it seems weird at first, but it looks to me the examples you made involve the *creation* of new objects, so it makes sense that the function, in case of errors, does not create them at all. In this case though the kobject_init_and_add is really just a shortcut for kobject_init + kobject_add and none of these functions create something for you. It's the caller's job to allocate the necessary memory, so I think that's the reason why it's still the caller that calls kobject_put when there's something wrong. Instead, in kobject_create_and_add, where the creation of the kobject is inside the function, the kobject is cleaned up if something wrong happens. Those are my thoughts anyway. Regards, Riccardo
On Tue, Nov 28, 2017 at 12:17:59AM +0100, Riccardo Schirone wrote: > Even when kobject_init_and_add/kset_register fail, the kobject has been > already initialized and the refcount set to 1. Thus it is necessary to > release the kobject/kset, to avoid the memory associated with it hanging > around forever. > > Signed-off-by: Riccardo Schirone <sirmy15@gmail.com> Thanks, applied. - Ted
diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c index 48c7a7d55ed3..b096e157934f 100644 --- a/fs/ext4/sysfs.c +++ b/fs/ext4/sysfs.c @@ -395,8 +395,11 @@ int ext4_register_sysfs(struct super_block *sb) init_completion(&sbi->s_kobj_unregister); err = kobject_init_and_add(&sbi->s_kobj, &ext4_sb_ktype, NULL, "%s", sb->s_id); - if (err) + if (err) { + kobject_put(&sbi->s_kobj); + wait_for_completion(&sbi->s_kobj_unregister); return err; + } if (ext4_proc_root) sbi->s_proc = proc_mkdir(sb->s_id, ext4_proc_root); @@ -429,15 +432,19 @@ int __init ext4_init_sysfs(void) kobject_set_name(&ext4_kset.kobj, "ext4"); ext4_kset.kobj.parent = fs_kobj; ret = kset_register(&ext4_kset); - if (ret) + if (ret) { + kset_unregister(&ext4_kset); return ret; + } ret = kobject_init_and_add(&ext4_feat, &ext4_feat_ktype, NULL, "features"); - if (ret) + if (ret) { + kobject_put(&ext4_feat); kset_unregister(&ext4_kset); - else + } else { ext4_proc_root = proc_mkdir(proc_dirname, NULL); + } return ret; }
Even when kobject_init_and_add/kset_register fail, the kobject has been already initialized and the refcount set to 1. Thus it is necessary to release the kobject/kset, to avoid the memory associated with it hanging around forever. Signed-off-by: Riccardo Schirone <sirmy15@gmail.com> --- fs/ext4/sysfs.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)