* [RFC] kobject_init changes @ 2007-11-30 19:51 Greg KH 2007-11-30 19:53 ` [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions Greg KH 0 siblings, 1 reply; 13+ messages in thread From: Greg KH @ 2007-11-30 19:51 UTC (permalink / raw) To: Cornelia Huck, Kay Sievers, Alan Stern Cc: Kernel development list, Jonathan Corbet, Randy Dunlap New thread time... Here are two patches, one that adds a two new functions, kobject_init_ng() and kobject_init_and_add() and then a patch that shows how a number of different users are converted over to these new functions. I choose the name "kobject_init_ng()" for now, so that I can do a patch series that converts the differnet usages in the kernel tree, and then do a global rename all at once, making it easier to find any potential problems that might happen. Same goes for the _and_add() function, which is really just kobject_register() but without the uevent call, and it has proper memory cleanup. Any objections to these changes? My plans with this are: - clean up the rest of the kernel tree usages of kobject_init() - rename kobject_init_ng() to kobject_init(). - remove all users of kobject_register() and use the new functions and a uevent call. - fix up the new kobject_create_and_register() function to be create_and_add() instead. The last one will require some patch reworking in my tree, but I have four days of meetings next week, and two plane rides, so I'll need something real to do :) I suppose if I really get bored, I can fix up the cdev stuff, but that's going to take a lot more work... thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions 2007-11-30 19:51 [RFC] kobject_init changes Greg KH @ 2007-11-30 19:53 ` Greg KH 2007-11-30 19:54 ` [RFC] kobject: convert some users of kobject_init to the new functions Greg KH 2007-11-30 20:25 ` [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions Alan Stern 0 siblings, 2 replies; 13+ messages in thread From: Greg KH @ 2007-11-30 19:53 UTC (permalink / raw) To: Cornelia Huck, Kay Sievers, Alan Stern Cc: Kernel development list, Jonathan Corbet, Randy Dunlap This is what the kobject_init function is going to become. Add it to the kernel and then we can convert over the current kobject_init() users before renaming it. Also add a kobject_init_and_add function which bundles up what a lot of the current callers want to do all at once, and it properly handles the memory usages, unlike kobject_register(); Cc: Kay Sievers <kay.sievers@vrfy.org> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> --- include/linux/kobject.h | 8 ++++ lib/kobject.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) --- a/include/linux/kobject.h +++ b/include/linux/kobject.h @@ -79,6 +79,14 @@ static inline const char * kobject_name( } extern void kobject_init(struct kobject *); +extern int __must_check kobject_init_ng(struct kobject *kobj, + struct kobj_type *ktype, + struct kobject *parent, + const char *fmt, ...); +extern int __must_check kobject_init_and_add(struct kobject *kobj, + struct kobj_type *ktype, + struct kobject *parent, + const char *fmt, ...); extern void kobject_cleanup(struct kobject *); extern int __must_check kobject_add(struct kobject *); --- a/lib/kobject.c +++ b/lib/kobject.c @@ -347,6 +347,97 @@ int kobject_set_name(struct kobject *kob } EXPORT_SYMBOL(kobject_set_name); +/* + * kobject_init_varg - the main kobject init function + * + * The kobject is initialized here with the needed fields. After this + * function returns successfully, the kobject can not be freed directly, + * kobject_put must be called in order to clean it up properly. If this + * function returns an error, the memory can be safely freed directly. + */ +static int kobject_init_varg(struct kobject *kobj, struct kobj_type *ktype, + struct kobject *parent, const char *fmt, + va_list vargs) +{ + va_list aq; + int retval; + + if ((!kobj) || (!ktype)) + return -EINVAL; + + WARN_ON(atomic_read(&kobj->kref.refcount)); + kref_init(&kobj->kref); + INIT_LIST_HEAD(&kobj->entry); + kobj->ktype = ktype; + kobj->parent = parent; + + va_copy(aq, vargs); + retval = kobject_set_name_vargs(kobj, fmt, aq); + va_end(aq); + + return retval; +} + +/** + * kobject_init_ng - initialize a kobject structure + * @kobj: pointer to the kobject to initialize + * @ktype: pointer to the ktype for this kobject. + * @parent: pointer to the parent of this kobject. + * @fmt: the name of the kobject. + * + * This function will properly initialize a kobject such that it can then + * be passed to the kobject_add() call. + * + * If the function returns an error, the memory allocated by the kobject + * can be safely freed, no other functions need to be called. + */ +int kobject_init_ng(struct kobject *kobj, struct kobj_type *ktype, + struct kobject *parent, const char *fmt, ...) +{ + va_list args; + int retval; + + va_start(args, fmt); + retval = kobject_init_varg(kobj, ktype, parent, fmt, args); + va_end(args); + + return retval; +} +EXPORT_SYMBOL(kobject_init_ng); + +/** + * kobject_init_and_add - initialize a kobject structure and add it to the kobject hierarchy + * @kobj: pointer to the kobject to initialize + * @ktype: pointer to the ktype for this kobject. + * @parent: pointer to the parent of this kobject. + * @fmt: the name of the kobject. + * + * This function will properly initialize a kobject and then call + * kobject_add(). + * + * If the function returns an error, the memory allocated by the kobject + * can be safely freed, no other functions need to be called. + */ +int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype, + struct kobject *parent, const char *fmt, ...) +{ + va_list args; + int retval; + + va_start(args, fmt); + retval = kobject_init_varg(kobj, ktype, parent, fmt, args); + va_end(args); + if (retval) + return retval; + + retval = kobject_add(kobj); + if (retval) + kobject_put(kobj); + + return retval; +} +EXPORT_SYMBOL(kobject_init_and_add); + /** * kobject_rename - change the name of an object * @kobj: object in question. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC] kobject: convert some users of kobject_init to the new functions. 2007-11-30 19:53 ` [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions Greg KH @ 2007-11-30 19:54 ` Greg KH 2007-11-30 20:25 ` [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions Alan Stern 1 sibling, 0 replies; 13+ messages in thread From: Greg KH @ 2007-11-30 19:54 UTC (permalink / raw) To: Cornelia Huck, Kay Sievers, Alan Stern Cc: Kernel development list, Jonathan Corbet, Randy Dunlap This converts the kernel/ directory and the char_dev directory to use the new kobject_init functions as an example and test of them. Cc: Kay Sievers <kay.sievers@vrfy.org> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> --- fs/char_dev.c | 18 ++++++++++++++---- kernel/module.c | 12 ++++-------- kernel/params.c | 5 +---- kernel/user.c | 5 +---- mm/slub.c | 5 +---- 5 files changed, 21 insertions(+), 24 deletions(-) --- a/fs/char_dev.c +++ b/fs/char_dev.c @@ -509,10 +509,15 @@ static struct kobj_type ktype_cdev_dynam struct cdev *cdev_alloc(void) { struct cdev *p = kzalloc(sizeof(struct cdev), GFP_KERNEL); + int ret; + if (p) { - p->kobj.ktype = &ktype_cdev_dynamic; INIT_LIST_HEAD(&p->list); - kobject_init(&p->kobj); + ret = kobject_init_ng(&p->kobj, &ktype_cdev_dynamic, NULL, + "cdev_dynamic"); + if (ret) + printk(KERN_ERR "cdev: something went wrong " + "initializing the dynamic kobject!\n"); } return p; } @@ -527,10 +532,15 @@ struct cdev *cdev_alloc(void) */ void cdev_init(struct cdev *cdev, const struct file_operations *fops) { + int ret; + memset(cdev, 0, sizeof *cdev); INIT_LIST_HEAD(&cdev->list); - cdev->kobj.ktype = &ktype_cdev_default; - kobject_init(&cdev->kobj); + ret = kobject_init_ng(&cdev->kobj, &ktype_cdev_default, NULL, + "cdev_static"); + if (ret) + printk(KERN_ERR "cdev: something went very wrong initializing " + "the static kobject!\n"); cdev->ops = fops; } --- a/kernel/module.c +++ b/kernel/module.c @@ -1218,18 +1218,14 @@ int mod_sysfs_init(struct module *mod) err = -EINVAL; goto out; } - memset(&mod->mkobj.kobj, 0, sizeof(mod->mkobj.kobj)); - err = kobject_set_name(&mod->mkobj.kobj, "%s", mod->name); - if (err) - goto out; - mod->mkobj.kobj.kset = module_kset; - mod->mkobj.kobj.ktype = &module_ktype; mod->mkobj.mod = mod; - kobject_init(&mod->mkobj.kobj); + memset(&mod->mkobj.kobj, 0, sizeof(mod->mkobj.kobj)); + mod->mkobj.kobj.kset = module_kset; + err = kobject_init_and_add(&mod->mkobj.kobj, &module_ktype, NULL, + "%s", mod->name); /* delay uevent until full sysfs population */ - err = kobject_add(&mod->mkobj.kobj); out: return err; } --- a/kernel/params.c +++ b/kernel/params.c @@ -561,10 +561,7 @@ static void __init kernel_param_sysfs_se mk->mod = THIS_MODULE; mk->kobj.kset = module_kset; - mk->kobj.ktype = &module_ktype; - kobject_set_name(&mk->kobj, name); - kobject_init(&mk->kobj); - ret = kobject_add(&mk->kobj); + ret = kobject_init_and_add(&mk->kobj, &module_ktype, NULL, "%s", name); if (ret) { printk(KERN_ERR "Module '%s' failed to be added to sysfs, " "error number %d\n", name, ret); --- a/kernel/user.c +++ b/kernel/user.c @@ -181,11 +181,8 @@ static int uids_user_create(struct user_ int error; memset(kobj, 0, sizeof(struct kobject)); - kobj->ktype = &uids_ktype; kobj->kset = uids_kset; - kobject_init(kobj); - kobject_set_name(&up->kobj, "%d", up->uid); - error = kobject_add(kobj); + error = kobject_init_and_add(kobj, &uids_ktype, NULL, "%d", up->uid); if (error) goto done; --- a/mm/slub.c +++ b/mm/slub.c @@ -4021,11 +4021,8 @@ static int sysfs_slab_add(struct kmem_ca name = create_unique_id(s); } - kobject_set_name(&s->kobj, name); s->kobj.kset = slab_kset; - s->kobj.ktype = &slab_ktype; - kobject_init(&s->kobj); - err = kobject_add(&s->kobj); + err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, name); if (err) return err; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions 2007-11-30 19:53 ` [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions Greg KH 2007-11-30 19:54 ` [RFC] kobject: convert some users of kobject_init to the new functions Greg KH @ 2007-11-30 20:25 ` Alan Stern 2007-11-30 21:04 ` Greg KH 1 sibling, 1 reply; 13+ messages in thread From: Alan Stern @ 2007-11-30 20:25 UTC (permalink / raw) To: Greg KH Cc: Cornelia Huck, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Fri, 30 Nov 2007, Greg KH wrote: > +/** > + * kobject_init_and_add - initialize a kobject structure and add it to the kobject hierarchy > + * @kobj: pointer to the kobject to initialize > + * @ktype: pointer to the ktype for this kobject. > + * @parent: pointer to the parent of this kobject. > + * @fmt: the name of the kobject. > + * > + * This function will properly initialize a kobject and then call > + * kobject_add(). > + * > + * If the function returns an error, the memory allocated by the kobject > + * can be safely freed, no other functions need to be called. > + */ > +int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype, > + struct kobject *parent, const char *fmt, ...) > +{ > + va_list args; > + int retval; > + > + va_start(args, fmt); > + retval = kobject_init_varg(kobj, ktype, parent, fmt, args); > + va_end(args); > + if (retval) > + return retval; > + > + retval = kobject_add(kobj); > + if (retval) > + kobject_put(kobj); No, no! You have recreated the problem we have been discussing during the last couple of days. If the kobject_init_varg() routine gets an error then the kobject will need to be deallocated manually. If the kobject_add() routine gets an error then the cleanup invoked by kobject_put() will do the deallocation automatically. But the caller can't tell in which subroutine an error occurred, so it won't know what to do when kobject_init_and_add() returns an error. The only way to resolve this problem is to have the _init routine consume no resources and never fail. That way the only possible failure mode would be if the _add routine doesn't work, in which case either a kfree() or a kobject_put() would be acceptable. In particular, this implies that the name should be set as part of the _add() call, not as part of _init(). This is more in line with the way the code tends to use kobjects anyhow. Unless people want to name unregistered kobjects -- does this ever happen? And it if does, can these kobjects simply be replaced by krefs? My suggestion: Have kobject_init_ng() accept a ktype pointer but not a parent or name. Instead, make kobject_add_ng() take the parent and name (possibly a kset also). Then when kobject_init_and_add() encounters an error, it shouldn't do a _put() -- the caller can either do the _put() or just do a kfree(). Alan Stern ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions 2007-11-30 20:25 ` [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions Alan Stern @ 2007-11-30 21:04 ` Greg KH 2007-11-30 21:07 ` Greg KH 2007-11-30 21:19 ` Alan Stern 0 siblings, 2 replies; 13+ messages in thread From: Greg KH @ 2007-11-30 21:04 UTC (permalink / raw) To: Alan Stern Cc: Cornelia Huck, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Fri, Nov 30, 2007 at 03:25:52PM -0500, Alan Stern wrote: > On Fri, 30 Nov 2007, Greg KH wrote: > > > +/** > > + * kobject_init_and_add - initialize a kobject structure and add it to the kobject hierarchy > > + * @kobj: pointer to the kobject to initialize > > + * @ktype: pointer to the ktype for this kobject. > > + * @parent: pointer to the parent of this kobject. > > + * @fmt: the name of the kobject. > > + * > > + * This function will properly initialize a kobject and then call > > + * kobject_add(). > > + * > > + * If the function returns an error, the memory allocated by the kobject > > + * can be safely freed, no other functions need to be called. > > + */ > > +int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype, > > + struct kobject *parent, const char *fmt, ...) > > +{ > > + va_list args; > > + int retval; > > + > > + va_start(args, fmt); > > + retval = kobject_init_varg(kobj, ktype, parent, fmt, args); > > + va_end(args); > > + if (retval) > > + return retval; > > + > > + retval = kobject_add(kobj); > > + if (retval) > > + kobject_put(kobj); > > No, no! > > You have recreated the problem we have been discussing during the last > couple of days. If the kobject_init_varg() routine gets an error then > the kobject will need to be deallocated manually. If the kobject_add() > routine gets an error then the cleanup invoked by kobject_put() will do > the deallocation automatically. > > But the caller can't tell in which subroutine an error occurred, so it > won't know what to do when kobject_init_and_add() returns an error. Oh crap. You're totally right. I suck. > The only way to resolve this problem is to have the _init routine > consume no resources and never fail. That way the only possible > failure mode would be if the _add routine doesn't work, in which case > either a kfree() or a kobject_put() would be acceptable. > > In particular, this implies that the name should be set as part of the > _add() call, not as part of _init(). This is more in line with the way > the code tends to use kobjects anyhow. Unless people want to name > unregistered kobjects -- does this ever happen? And it if does, can > these kobjects simply be replaced by krefs? No, the only non-registered kobjects in the tree right now are never named. So this should be safe. > My suggestion: Have kobject_init_ng() accept a ktype pointer but not a > parent or name. Instead, make kobject_add_ng() take the parent and > name (possibly a kset also). Then when kobject_init_and_add() > encounters an error, it shouldn't do a _put() -- the caller can either > do the _put() or just do a kfree(). Why not the parent for init()? Isn't it always known at that time? I'll dig to be sure. Ok, second round of patches coming up... thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions 2007-11-30 21:04 ` Greg KH @ 2007-11-30 21:07 ` Greg KH 2007-11-30 21:19 ` Alan Stern 1 sibling, 0 replies; 13+ messages in thread From: Greg KH @ 2007-11-30 21:07 UTC (permalink / raw) To: Alan Stern Cc: Cornelia Huck, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Fri, Nov 30, 2007 at 01:04:20PM -0800, Greg KH wrote: > > My suggestion: Have kobject_init_ng() accept a ktype pointer but not a > > parent or name. Instead, make kobject_add_ng() take the parent and > > name (possibly a kset also). Then when kobject_init_and_add() > > encounters an error, it shouldn't do a _put() -- the caller can either > > do the _put() or just do a kfree(). > > Why not the parent for init()? Isn't it always known at that time? > I'll dig to be sure. You are right again, to match up with the driver core methods, we don't know the parent until add() time. thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions 2007-11-30 21:04 ` Greg KH 2007-11-30 21:07 ` Greg KH @ 2007-11-30 21:19 ` Alan Stern 2007-11-30 21:48 ` Greg KH 1 sibling, 1 reply; 13+ messages in thread From: Alan Stern @ 2007-11-30 21:19 UTC (permalink / raw) To: Greg KH Cc: Cornelia Huck, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Fri, 30 Nov 2007, Greg KH wrote: > > My suggestion: Have kobject_init_ng() accept a ktype pointer but not a > > parent or name. Instead, make kobject_add_ng() take the parent and > > name (possibly a kset also). Then when kobject_init_and_add() > > encounters an error, it shouldn't do a _put() -- the caller can either > > do the _put() or just do a kfree(). > > Why not the parent for init()? Isn't it always known at that time? > I'll dig to be sure. Specifying the parent during _add() is more logical, because a kobject doesn't actually _do_ anything to the parent until it is registered in the parent's directory. Or to put it another way, an unregistered kobject can't have a parent in any meaningful sense so there's no point specifying the parent in the _init() call. It's really just a matter of taste. Alan Stern ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions 2007-11-30 21:19 ` Alan Stern @ 2007-11-30 21:48 ` Greg KH 2007-11-30 22:10 ` Alan Stern 0 siblings, 1 reply; 13+ messages in thread From: Greg KH @ 2007-11-30 21:48 UTC (permalink / raw) To: Alan Stern Cc: Cornelia Huck, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Fri, Nov 30, 2007 at 04:19:53PM -0500, Alan Stern wrote: > On Fri, 30 Nov 2007, Greg KH wrote: > > > > My suggestion: Have kobject_init_ng() accept a ktype pointer but not a > > > parent or name. Instead, make kobject_add_ng() take the parent and > > > name (possibly a kset also). Then when kobject_init_and_add() > > > encounters an error, it shouldn't do a _put() -- the caller can either > > > do the _put() or just do a kfree(). > > > > Why not the parent for init()? Isn't it always known at that time? > > I'll dig to be sure. > > Specifying the parent during _add() is more logical, because a kobject > doesn't actually _do_ anything to the parent until it is registered in > the parent's directory. Or to put it another way, an unregistered > kobject can't have a parent in any meaningful sense so there's no point > specifying the parent in the _init() call. Ok, how about this: void kobject_init(struct kobject *kobj, struct ktype *ktype); and then: int kobject_add(struct kobject *kobj, struct kobject *parent, const char *fmt, ...); After we call kobject_init() we HAVE to call kobject_put() to clean up properly. So, if kobject_add() fails, we still need to clean up with kobject_put(); That means we _can_ create a: int kobject_init_and_add(struct kobject *kobj, struct ktype *ktype, struct kobject *parent, const char *fmt, ...); and if that fails, then again, you have to call kobject_put() to clean things up, right? Does this look sane? thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions 2007-11-30 21:48 ` Greg KH @ 2007-11-30 22:10 ` Alan Stern 2007-11-30 22:26 ` Greg KH 2007-11-30 22:33 ` Greg KH 0 siblings, 2 replies; 13+ messages in thread From: Alan Stern @ 2007-11-30 22:10 UTC (permalink / raw) To: Greg KH Cc: Cornelia Huck, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Fri, 30 Nov 2007, Greg KH wrote: > Ok, how about this: > void kobject_init(struct kobject *kobj, struct ktype *ktype); > > and then: > int kobject_add(struct kobject *kobj, struct kobject *parent, const char *fmt, ...); > > After we call kobject_init() we HAVE to call kobject_put() to clean up > properly. So, if kobject_add() fails, we still need to clean up with > kobject_put(); You could put that a little less strongly. After kobject_init() you SHOULD call kobject_put() to clean up properly, and after kobject_add() you MUST call kobject_del() and kobject_put(). However if kobject_add() is never called, or if it is called and it fails, then it's okay to use kfree(). It's not clear whether this distinction will matter in practice. It's probably best to document this using your stronger description. The same sort of rule should apply to other kernel objects, like struct device. After intialization you have to do a final _put, before that you just do a kfree(). (And initialization cannot fail.) > That means we _can_ create a: > int kobject_init_and_add(struct kobject *kobj, struct ktype *ktype, struct kobject *parent, const char *fmt, ...); > > and if that fails, then again, you have to call kobject_put() to clean > things up, right? Right. Because you know that the failure must have occurred in the _add portion. > Does this look sane? Yes, I think that will all work correctly. Alan Stern ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions 2007-11-30 22:10 ` Alan Stern @ 2007-11-30 22:26 ` Greg KH 2007-11-30 23:22 ` Alan Stern 2007-11-30 22:33 ` Greg KH 1 sibling, 1 reply; 13+ messages in thread From: Greg KH @ 2007-11-30 22:26 UTC (permalink / raw) To: Alan Stern Cc: Cornelia Huck, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Fri, Nov 30, 2007 at 05:10:33PM -0500, Alan Stern wrote: > On Fri, 30 Nov 2007, Greg KH wrote: > > > Ok, how about this: > > void kobject_init(struct kobject *kobj, struct ktype *ktype); > > > > and then: > > int kobject_add(struct kobject *kobj, struct kobject *parent, const char *fmt, ...); > > > > After we call kobject_init() we HAVE to call kobject_put() to clean up > > properly. So, if kobject_add() fails, we still need to clean up with > > kobject_put(); > > You could put that a little less strongly. After kobject_init() you > SHOULD call kobject_put() to clean up properly, and after kobject_add() > you MUST call kobject_del() and kobject_put(). > > However if kobject_add() is never called, or if it is called and it > fails, then it's okay to use kfree(). It's not clear whether this > distinction will matter in practice. It's probably best to document > this using your stronger description. No, if kobject_add() fails, kobject_put() still must be called in order to free up the name pointer, unless you are somehow guessing that the "kobject_set_name()" portion of kobject_add() somehow failed. And you can't know that, so you have to call kobject_put() in order to be safe and clean up everything. Now why did we not do the final kobject_put() in kobject_del() as well? Doing two calls, always in order, seems a bit strange, anyone know why it's this way? > The same sort of rule should apply to other kernel objects, like struct > device. After intialization you have to do a final _put, before that > you just do a kfree(). (And initialization cannot fail.) Yes. > > That means we _can_ create a: > > int kobject_init_and_add(struct kobject *kobj, struct ktype *ktype, struct kobject *parent, const char *fmt, ...); > > > > and if that fails, then again, you have to call kobject_put() to clean > > things up, right? > > Right. Because you know that the failure must have occurred in the > _add portion. Ok, good, I might get this right yet :) thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions 2007-11-30 22:26 ` Greg KH @ 2007-11-30 23:22 ` Alan Stern 2007-12-01 0:58 ` Greg KH 0 siblings, 1 reply; 13+ messages in thread From: Alan Stern @ 2007-11-30 23:22 UTC (permalink / raw) To: Greg KH Cc: Cornelia Huck, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Fri, 30 Nov 2007, Greg KH wrote: > > However if kobject_add() is never called, or if it is called and it > > fails, then it's okay to use kfree(). It's not clear whether this > > distinction will matter in practice. It's probably best to document > > this using your stronger description. > > No, if kobject_add() fails, kobject_put() still must be called in order > to free up the name pointer, unless you are somehow guessing that the > "kobject_set_name()" portion of kobject_add() somehow failed. Actually I imagined that if kobject_add() failed it would back out all the changes it made -- which means it would deallocate the name string. But requiring people to call kobject_put() will do this just as well. > And you > can't know that, so you have to call kobject_put() in order to be safe > and clean up everything. > > Now why did we not do the final kobject_put() in kobject_del() as well? > Doing two calls, always in order, seems a bit strange, anyone know why > it's this way? To be symmetrical with kobject_init() and kobject_add(). Besides, isn't there kobject_unregister()? Presumably it will go away along with kobject_register(), though. > > You could put that a little less strongly. After kobject_init() you > > SHOULD call kobject_put() to clean up properly, and after kobject_add() > > you MUST call kobject_del() and kobject_put(). > > No, in looking at the code, you only need to call kobject_del() to clean > everything up properly, if kobject_add() succeeds. No need to call > kobject_put() yet again. Sorry, yes, that's what I meant. After a successful call to kobject_add() you must call kobject_del() to undo the _add, and then kobject_put() for the final cleanup. Alan Stern ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions 2007-11-30 23:22 ` Alan Stern @ 2007-12-01 0:58 ` Greg KH 0 siblings, 0 replies; 13+ messages in thread From: Greg KH @ 2007-12-01 0:58 UTC (permalink / raw) To: Alan Stern Cc: Cornelia Huck, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Fri, Nov 30, 2007 at 06:22:37PM -0500, Alan Stern wrote: > On Fri, 30 Nov 2007, Greg KH wrote: > > And you > > can't know that, so you have to call kobject_put() in order to be safe > > and clean up everything. > > > > Now why did we not do the final kobject_put() in kobject_del() as well? > > Doing two calls, always in order, seems a bit strange, anyone know why > > it's this way? > > To be symmetrical with kobject_init() and kobject_add(). Besides, > isn't there kobject_unregister()? Presumably it will go away along > with kobject_register(), though. Yes, it will go away too, once everyone gets converted. > > > You could put that a little less strongly. After kobject_init() you > > > SHOULD call kobject_put() to clean up properly, and after kobject_add() > > > you MUST call kobject_del() and kobject_put(). > > > > No, in looking at the code, you only need to call kobject_del() to clean > > everything up properly, if kobject_add() succeeds. No need to call > > kobject_put() yet again. > > Sorry, yes, that's what I meant. After a successful call to > kobject_add() you must call kobject_del() to undo the _add, and then > kobject_put() for the final cleanup. No, kobject_del() does the put for you[1]. All that is needed is a call to kobject_del(). I'll post the updated patches in a minute, they look and seem to work much better. thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions 2007-11-30 22:10 ` Alan Stern 2007-11-30 22:26 ` Greg KH @ 2007-11-30 22:33 ` Greg KH 1 sibling, 0 replies; 13+ messages in thread From: Greg KH @ 2007-11-30 22:33 UTC (permalink / raw) To: Alan Stern Cc: Cornelia Huck, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Fri, Nov 30, 2007 at 05:10:33PM -0500, Alan Stern wrote: > On Fri, 30 Nov 2007, Greg KH wrote: > > > Ok, how about this: > > void kobject_init(struct kobject *kobj, struct ktype *ktype); > > > > and then: > > int kobject_add(struct kobject *kobj, struct kobject *parent, const char *fmt, ...); > > > > After we call kobject_init() we HAVE to call kobject_put() to clean up > > properly. So, if kobject_add() fails, we still need to clean up with > > kobject_put(); > > You could put that a little less strongly. After kobject_init() you > SHOULD call kobject_put() to clean up properly, and after kobject_add() > you MUST call kobject_del() and kobject_put(). No, in looking at the code, you only need to call kobject_del() to clean everything up properly, if kobject_add() succeeds. No need to call kobject_put() yet again. Can someone else verify that this really is correct? thanks, greg k-h p.s. I think it's time to start a "travel to .nz and kick a certain ex-kernel-developer around a bit" fund... ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2007-12-01 0:59 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-11-30 19:51 [RFC] kobject_init changes Greg KH 2007-11-30 19:53 ` [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions Greg KH 2007-11-30 19:54 ` [RFC] kobject: convert some users of kobject_init to the new functions Greg KH 2007-11-30 20:25 ` [RFC] kobject: add kobject_init_ng and kobject_init_and_add functions Alan Stern 2007-11-30 21:04 ` Greg KH 2007-11-30 21:07 ` Greg KH 2007-11-30 21:19 ` Alan Stern 2007-11-30 21:48 ` Greg KH 2007-11-30 22:10 ` Alan Stern 2007-11-30 22:26 ` Greg KH 2007-11-30 23:22 ` Alan Stern 2007-12-01 0:58 ` Greg KH 2007-11-30 22:33 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).