* [RFC] kobject_init changes - take 2 @ 2007-12-01 1:00 Greg KH 2007-12-01 1:01 ` [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions Greg KH 0 siblings, 1 reply; 10+ messages in thread From: Greg KH @ 2007-12-01 1:00 UTC (permalink / raw) To: Cornelia Huck, Kay Sievers, Alan Stern Cc: Kernel development list, Jonathan Corbet, Randy Dunlap After Alan pointed out my stupidity, here are some new patches :) They add three new functions: kobject_init_ng() kobject_add_ng() kobject_init_and_add() The "_ng" portion will go away after all of the current kernel users of kobject_init() and kobject_add() are converted over. There's also a second patch that shows how they are used, and how this actually saves code in the callers. Any further objections about these changes? thanks, greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions 2007-12-01 1:00 [RFC] kobject_init changes - take 2 Greg KH @ 2007-12-01 1:01 ` Greg KH 2007-12-01 1:02 ` [RFC] kobject: the new functions in use Greg KH 2007-12-01 3:29 ` [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions Alan Stern 0 siblings, 2 replies; 10+ messages in thread From: Greg KH @ 2007-12-01 1:01 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 | 11 ++- lib/kobject.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 156 insertions(+), 7 deletions(-) --- a/include/linux/kobject.h +++ b/include/linux/kobject.h @@ -79,9 +79,16 @@ static inline const char * kobject_name( } extern void kobject_init(struct kobject *); -extern void kobject_cleanup(struct kobject *); - +extern void kobject_init_ng(struct kobject *kobj, struct kobj_type *ktype); extern int __must_check kobject_add(struct kobject *); +extern int __must_check kobject_add_ng(struct kobject *kobj, + 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_del(struct kobject *); extern struct kobject * __must_check kobject_create(const char *name, --- a/lib/kobject.c +++ b/lib/kobject.c @@ -348,6 +348,149 @@ int kobject_set_name(struct kobject *kob EXPORT_SYMBOL(kobject_set_name); /** + * 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. + */ +void kobject_init_ng(struct kobject *kobj, struct kobj_type *ktype) +{ + char *err_str; + + if (!kobj) { + err_str = "invalid kobject pointer!"; + goto error; + } + if (!ktype) { + err_str = "must have a ktype to be initialized properly!\n"; + goto error; + } + if (atomic_read(&kobj->kref.refcount)) { + /* do not error out as sometimes we can recover */ + printk(KERN_ERR "kobject: reference count is already set, " + "something is seriously wrong.\n"); + dump_stack(); + } + + kref_init(&kobj->kref); + INIT_LIST_HEAD(&kobj->entry); + kobj->ktype = ktype; + return; + +error: + printk(KERN_ERR "kobject: %s\n", err_str); + dump_stack(); +} +EXPORT_SYMBOL(kobject_init_ng); + +static int kobject_add_varg(struct kobject *kobj, struct kobject *parent, + const char *fmt, va_list vargs) +{ + va_list aq; + int retval; + + va_copy(aq, vargs); + retval = kobject_set_name_vargs(kobj, fmt, aq); + va_end(aq); + if (retval) { + printk(KERN_ERR "kobject: can not set name properly!\n"); + return retval; + } + kobj->parent = parent; + return kobject_add(kobj); +} + +/** + * kobject_add_ng - the main kobject add function + * @kobj: the kobject to add + * @parent: pointer to the parent of the kobject. + * + * The kobject name is set and added to the kobject hierarchy in this + * function. + * + * If @parent is set, then the parent of the @kobj will be set to it. + * If @parent is NULL, then the parent of the @kobj will be set to the + * kobject associted with the kset assigned to this kobject. If no kset + * is assigned to the kobject, then the kobject will be located in the + * root of the sysfs tree. + * + * If this function returns an error, kobject_put() must be called to + * properly clean up the memory associated with the object. + * + * If the function is successful, the only way to properly clean up the + * memory is with a call to kobject_del(). + * + * Under no instance should the kobject that is passed to this function + * be directly freed with a call to kfree(), that can leak memory. + */ +int kobject_add_ng(struct kobject *kobj, struct kobject *parent, + const char *fmt, ...) +{ + va_list args; + int retval; + + if (!kobj) + return -EINVAL; + + va_start(args, fmt); + retval = kobject_set_name_vargs(kobj, fmt, args); + va_end(args); + if (retval) { + printk(KERN_ERR "kobject: can not set name properly!\n"); + return retval; + } + kobj->parent = parent; + return kobject_add(kobj); +} +EXPORT_SYMBOL(kobject_add_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 kobject passed to this function + * must be cleaned up by calling kobject_put(), and not by directly + * trying to call kfree() on the kobject. + * + * If this function succeeds, the only way to properly clean up the + * kobject is to call kobject_destroy(), which will clean up all of the + * needed sysfs objects, and the kobject itself (by calling back to the + * ktype->release() function.) + * + * Note that the kobject_uevent() call should be called after this + * function succeeds, so that userspace can properly know that the + * kobject was created. + */ +int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype, + struct kobject *parent, const char *fmt, ...) +{ + va_list args; + int retval; + + kobject_init_ng(kobj, ktype); + + va_start(args, fmt); + retval = kobject_add_varg(kobj, parent, fmt, args); + va_end(args); + + return retval; +} +EXPORT_SYMBOL(kobject_init_and_add); + +/** * kobject_rename - change the name of an object * @kobj: object in question. * @new_name: object's new name @@ -502,12 +645,11 @@ struct kobject * kobject_get(struct kobj return kobj; } -/** - * kobject_cleanup - free kobject resources. - * @kobj: object. +/* + * kobject_cleanup - free kobject resources. + * @kobj: object. */ - -void kobject_cleanup(struct kobject * kobj) +static void kobject_cleanup(struct kobject *kobj) { struct kobj_type * t = get_ktype(kobj); struct kset * s = kobj->kset; ^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC] kobject: the new functions in use 2007-12-01 1:01 ` [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions Greg KH @ 2007-12-01 1:02 ` Greg KH 2007-12-01 3:29 ` [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions Alan Stern 1 sibling, 0 replies; 10+ messages in thread From: Greg KH @ 2007-12-01 1:02 UTC (permalink / raw) To: Cornelia Huck, Kay Sievers, Alan Stern Cc: Kernel development list, Jonathan Corbet, Randy Dunlap Here's an example of a number of different places in the kernel that have been converted from the older kobject functions to the new versions. Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> --- fs/char_dev.c | 6 ++---- kernel/module.c | 14 ++++++-------- kernel/params.c | 6 ++---- kernel/user.c | 9 ++++----- mm/slub.c | 9 ++++----- net/bridge/br_if.c | 10 +++------- 6 files changed, 21 insertions(+), 33 deletions(-) --- a/fs/char_dev.c +++ b/fs/char_dev.c @@ -510,9 +510,8 @@ struct cdev *cdev_alloc(void) { struct cdev *p = kzalloc(sizeof(struct cdev), GFP_KERNEL); if (p) { - p->kobj.ktype = &ktype_cdev_dynamic; INIT_LIST_HEAD(&p->list); - kobject_init(&p->kobj); + kobject_init_ng(&p->kobj, &ktype_cdev_dynamic); } return p; } @@ -529,8 +528,7 @@ void cdev_init(struct cdev *cdev, const { memset(cdev, 0, sizeof *cdev); INIT_LIST_HEAD(&cdev->list); - cdev->kobj.ktype = &ktype_cdev_default; - kobject_init(&cdev->kobj); + kobject_init_ng(&cdev->kobj, &ktype_cdev_default); cdev->ops = fops; } --- a/kernel/module.c +++ b/kernel/module.c @@ -1218,18 +1218,16 @@ 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); + if (err) + kobject_put(&mod->mkobj.kobj); /* delay uevent until full sysfs population */ - err = kobject_add(&mod->mkobj.kobj); out: return err; } --- a/kernel/params.c +++ b/kernel/params.c @@ -561,11 +561,9 @@ 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) { + kobject_put(&mk->kobj); printk(KERN_ERR "Module '%s' failed to be added to sysfs, " "error number %d\n", name, ret); printk(KERN_ERR "The system will be unstable now.\n"); --- a/kernel/user.c +++ b/kernel/user.c @@ -181,13 +181,12 @@ 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); - if (error) + error = kobject_init_and_add(kobj, &uids_ktype, NULL, "%d", up->uid); + if (error) { + kobject_put(kobj); goto done; + } kobject_uevent(kobj, KOBJ_ADD); done: --- a/mm/slub.c +++ b/mm/slub.c @@ -4021,13 +4021,12 @@ 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); - if (err) + err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, name); + if (err) { + kobject_put(&s->kobj); return err; + } err = sysfs_create_group(&s->kobj, &slab_attr_group); if (err) --- a/net/bridge/br_if.c +++ b/net/bridge/br_if.c @@ -258,12 +258,6 @@ static struct net_bridge_port *new_nbp(s p->state = BR_STATE_DISABLED; br_stp_port_timer_init(p); - kobject_init(&p->kobj); - kobject_set_name(&p->kobj, SYSFS_BRIDGE_PORT_ATTR); - p->kobj.ktype = &brport_ktype; - p->kobj.parent = &(dev->dev.kobj); - p->kobj.kset = NULL; - return p; } @@ -379,7 +373,8 @@ int br_add_if(struct net_bridge *br, str if (IS_ERR(p)) return PTR_ERR(p); - err = kobject_add(&p->kobj); + err = kobject_init_and_add(&p->kobj, &brport_ktype, &(dev->dev.kobj), + SYSFS_BRIDGE_PORT_ATTR); if (err) goto err0; @@ -416,6 +411,7 @@ err2: br_fdb_delete_by_port(br, p, 1); err1: kobject_del(&p->kobj); + return err; err0: kobject_put(&p->kobj); return err; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions 2007-12-01 1:01 ` [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions Greg KH 2007-12-01 1:02 ` [RFC] kobject: the new functions in use Greg KH @ 2007-12-01 3:29 ` Alan Stern 2007-12-03 12:00 ` Cornelia Huck 2007-12-03 19:05 ` Greg KH 1 sibling, 2 replies; 10+ messages in thread From: Alan Stern @ 2007-12-01 3:29 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_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. > + */ > +void kobject_init_ng(struct kobject *kobj, struct kobj_type *ktype) Kerneldoc needs to be updated -- no @parent or @fmt. Also no error returns. But you could say that after this routine runs, the kobject should be deallocated by kobject_put() and not by calling kfree() directly. > +/** > + * kobject_add_ng - the main kobject add function > + * @kobj: the kobject to add > + * @parent: pointer to the parent of the kobject. > + * > + * The kobject name is set and added to the kobject hierarchy in this > + * function. > + * > + * If @parent is set, then the parent of the @kobj will be set to it. > + * If @parent is NULL, then the parent of the @kobj will be set to the > + * kobject associted with the kset assigned to this kobject. If no kset > + * is assigned to the kobject, then the kobject will be located in the > + * root of the sysfs tree. > + * > + * If this function returns an error, kobject_put() must be called to > + * properly clean up the memory associated with the object. > + * > + * If the function is successful, the only way to properly clean up the > + * memory is with a call to kobject_del(). In which case kobject_put() isn't needed? > + * > + * Under no instance should the kobject that is passed to this function > + * be directly freed with a call to kfree(), that can leak memory. > + */ Should you say something here about uevents? > +int kobject_add_ng(struct kobject *kobj, struct kobject *parent, > + const char *fmt, ...) > +{ > + va_list args; > + int retval; > + > + if (!kobj) > + return -EINVAL; > + > + va_start(args, fmt); > + retval = kobject_set_name_vargs(kobj, fmt, args); > + va_end(args); > + if (retval) { > + printk(KERN_ERR "kobject: can not set name properly!\n"); > + return retval; > + } > + kobj->parent = parent; > + return kobject_add(kobj); > +} > +EXPORT_SYMBOL(kobject_add_ng); Looks like this should call kobject_add_varg() instead of duplicating its code. > +/** > + * 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 kobject passed to this function > + * must be cleaned up by calling kobject_put(), and not by directly > + * trying to call kfree() on the kobject. > + * > + * If this function succeeds, the only way to properly clean up the > + * kobject is to call kobject_destroy(), which will clean up all of the kobject_destroy()? Where did that come from? Or did you mean kobject_del()? > + * needed sysfs objects, and the kobject itself (by calling back to the > + * ktype->release() function.) > + * > + * Note that the kobject_uevent() call should be called after this > + * function succeeds, so that userspace can properly know that the > + * kobject was created. > + */ Could the comments be made shorter by saying merely that this routine combines calls to kobject_init() and kobject_add_ng()? > +int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype, > + struct kobject *parent, const char *fmt, ...) > +{ > + va_list args; > + int retval; > + > + kobject_init_ng(kobj, ktype); > + > + va_start(args, fmt); > + retval = kobject_add_varg(kobj, parent, fmt, args); > + va_end(args); > + > + return retval; > +} > +EXPORT_SYMBOL(kobject_init_and_add); Looks okay. Did you want to add an extra kobject_put() to the end of kobject_del()? Or did you want to define a new kobject_destroy() that combines calls to kobject_del() and kobject_put()? Alan Stern ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions 2007-12-01 3:29 ` [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions Alan Stern @ 2007-12-03 12:00 ` Cornelia Huck 2007-12-03 19:06 ` Greg KH 2007-12-03 19:05 ` Greg KH 1 sibling, 1 reply; 10+ messages in thread From: Cornelia Huck @ 2007-12-03 12:00 UTC (permalink / raw) To: Alan Stern Cc: Greg KH, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Fri, 30 Nov 2007 22:29:39 -0500 (EST), Alan Stern <stern@rowland.harvard.edu> wrote: > On Fri, 30 Nov 2007, Greg KH wrote: > > > /** > > + * 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. > > + */ > > +void kobject_init_ng(struct kobject *kobj, struct kobj_type *ktype) > > Kerneldoc needs to be updated -- no @parent or @fmt. Also no error > returns. But you could say that after this routine runs, the kobject > should be deallocated by kobject_put() and not by calling kfree() > directly. Hm, after calling kobject_init_ng() is also the earliest point that you can rely on kobject_put() really cleaning stuff up. Both kobject_put() and kfree() are fine then, but I think kobject_put() makes for cleaner code. > > > +/** > > + * kobject_add_ng - the main kobject add function > > + * @kobj: the kobject to add > > + * @parent: pointer to the parent of the kobject. > > + * > > + * The kobject name is set and added to the kobject hierarchy in this > > + * function. > > + * > > + * If @parent is set, then the parent of the @kobj will be set to it. > > + * If @parent is NULL, then the parent of the @kobj will be set to the > > + * kobject associted with the kset assigned to this kobject. If no kset > > + * is assigned to the kobject, then the kobject will be located in the > > + * root of the sysfs tree. > > + * > > + * If this function returns an error, kobject_put() must be called to > > + * properly clean up the memory associated with the object. > > + * > > + * If the function is successful, the only way to properly clean up the > > + * memory is with a call to kobject_del(). > > In which case kobject_put() isn't needed? kobject_del() should only undo what kobject_add() did. So kobject_put() will still be needed to clean up the memory. Perhaps the wording should be: If the function is successful, the only way to properly clean up the kobject is to call kobject_del() for removing the kobject from the hierarchy and to subsequently call kobject_put() to clean up the memory. > > > + * > > + * Under no instance should the kobject that is passed to this function > > + * be directly freed with a call to kfree(), that can leak memory. > > + */ > > Should you say something here about uevents? Probably not. Callers of kobject_add() always had to create the uevent themselves; it was only with kobject_register() they could rely on the uevent being created. > > > +int kobject_add_ng(struct kobject *kobj, struct kobject *parent, > > + const char *fmt, ...) > > +{ > > + va_list args; > > + int retval; > > + > > + if (!kobj) > > + return -EINVAL; > > + > > + va_start(args, fmt); > > + retval = kobject_set_name_vargs(kobj, fmt, args); > > + va_end(args); > > + if (retval) { > > + printk(KERN_ERR "kobject: can not set name properly!\n"); > > + return retval; > > + } > > + kobj->parent = parent; > > + return kobject_add(kobj); > > +} > > +EXPORT_SYMBOL(kobject_add_ng); > > Looks like this should call kobject_add_varg() instead of duplicating > its code. Agreed. And how about "cannot" or "could not" instead of "can not"? > > > +/** > > + * 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 kobject passed to this function > > + * must be cleaned up by calling kobject_put(), and not by directly > > + * trying to call kfree() on the kobject. > > + * > > + * If this function succeeds, the only way to properly clean up the > > + * kobject is to call kobject_destroy(), which will clean up all of the > > kobject_destroy()? Where did that come from? Or did you mean > kobject_del()? This sentence makes only sense if kobject_destroy() is something like kobject_unregister_ng(). > > > + * needed sysfs objects, and the kobject itself (by calling back to the > > + * ktype->release() function.) > > + * > > + * Note that the kobject_uevent() call should be called after this > > + * function succeeds, so that userspace can properly know that the > > + * kobject was created. > > + */ > > Could the comments be made shorter by saying merely that this routine > combines calls to kobject_init() and kobject_add_ng()? I think it should be made explicit which actions are OK after failure or success of this function since that is what people easily get wrong. > > > +int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype, > > + struct kobject *parent, const char *fmt, ...) > > +{ > > + va_list args; > > + int retval; > > + > > + kobject_init_ng(kobj, ktype); > > + > > + va_start(args, fmt); > > + retval = kobject_add_varg(kobj, parent, fmt, args); > > + va_end(args); > > + > > + return retval; > > +} > > +EXPORT_SYMBOL(kobject_init_and_add); > > Looks okay. Agreed. > > Did you want to add an extra kobject_put() to the end of kobject_del()? This would be surprising: I wouldn't expect a kobject to be cleaned up just because I removed it from the hierarchy. > Or did you want to define a new kobject_destroy() that combines calls > to kobject_del() and kobject_put()? This looks saner. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions 2007-12-03 12:00 ` Cornelia Huck @ 2007-12-03 19:06 ` Greg KH 2007-12-04 10:01 ` Cornelia Huck 0 siblings, 1 reply; 10+ messages in thread From: Greg KH @ 2007-12-03 19:06 UTC (permalink / raw) To: Cornelia Huck Cc: Alan Stern, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Mon, Dec 03, 2007 at 01:00:03PM +0100, Cornelia Huck wrote: > On Fri, 30 Nov 2007 22:29:39 -0500 (EST), > Alan Stern <stern@rowland.harvard.edu> wrote: > > > On Fri, 30 Nov 2007, Greg KH wrote: > > > > > +/** > > > + * kobject_add_ng - the main kobject add function > > > + * @kobj: the kobject to add > > > + * @parent: pointer to the parent of the kobject. > > > + * > > > + * The kobject name is set and added to the kobject hierarchy in this > > > + * function. > > > + * > > > + * If @parent is set, then the parent of the @kobj will be set to it. > > > + * If @parent is NULL, then the parent of the @kobj will be set to the > > > + * kobject associted with the kset assigned to this kobject. If no kset > > > + * is assigned to the kobject, then the kobject will be located in the > > > + * root of the sysfs tree. > > > + * > > > + * If this function returns an error, kobject_put() must be called to > > > + * properly clean up the memory associated with the object. > > > + * > > > + * If the function is successful, the only way to properly clean up the > > > + * memory is with a call to kobject_del(). > > > > In which case kobject_put() isn't needed? > > kobject_del() should only undo what kobject_add() did. So kobject_put() > will still be needed to clean up the memory. Perhaps the wording should > be: > > If the function is successful, the only way to properly clean up the > kobject is to call kobject_del() for removing the kobject from the > hierarchy and to subsequently call kobject_put() to clean up the memory. But that's not what the code does today in the kobject_del() function. So for now, let's leave what we have. thanks, greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions 2007-12-03 19:06 ` Greg KH @ 2007-12-04 10:01 ` Cornelia Huck 2007-12-04 15:28 ` Alan Stern 0 siblings, 1 reply; 10+ messages in thread From: Cornelia Huck @ 2007-12-04 10:01 UTC (permalink / raw) To: Greg KH Cc: Alan Stern, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Mon, 3 Dec 2007 11:06:26 -0800, Greg KH <greg@kroah.com> wrote: > On Mon, Dec 03, 2007 at 01:00:03PM +0100, Cornelia Huck wrote: > > On Fri, 30 Nov 2007 22:29:39 -0500 (EST), > > Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > On Fri, 30 Nov 2007, Greg KH wrote: > > > > > > > +/** > > > > + * kobject_add_ng - the main kobject add function > > > > + * @kobj: the kobject to add > > > > + * @parent: pointer to the parent of the kobject. > > > > + * > > > > + * The kobject name is set and added to the kobject hierarchy in this > > > > + * function. > > > > + * > > > > + * If @parent is set, then the parent of the @kobj will be set to it. > > > > + * If @parent is NULL, then the parent of the @kobj will be set to the > > > > + * kobject associted with the kset assigned to this kobject. If no kset > > > > + * is assigned to the kobject, then the kobject will be located in the > > > > + * root of the sysfs tree. > > > > + * > > > > + * If this function returns an error, kobject_put() must be called to > > > > + * properly clean up the memory associated with the object. > > > > + * > > > > + * If the function is successful, the only way to properly clean up the > > > > + * memory is with a call to kobject_del(). > > > > > > In which case kobject_put() isn't needed? > > > > kobject_del() should only undo what kobject_add() did. So kobject_put() > > will still be needed to clean up the memory. Perhaps the wording should > > be: > > > > If the function is successful, the only way to properly clean up the > > kobject is to call kobject_del() for removing the kobject from the > > hierarchy and to subsequently call kobject_put() to clean up the memory. > > But that's not what the code does today in the kobject_del() function. Hm, if I'm not completely confused, kobject_del() just gives up the extra reference obtained by kobject_add(). That leaves the initial reference we got via kobject_init(_ng)(). If we want to clean up the memory, we need to give up that reference as well. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions 2007-12-04 10:01 ` Cornelia Huck @ 2007-12-04 15:28 ` Alan Stern 0 siblings, 0 replies; 10+ messages in thread From: Alan Stern @ 2007-12-04 15:28 UTC (permalink / raw) To: Cornelia Huck Cc: Greg KH, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Tue, 4 Dec 2007, Cornelia Huck wrote: > > > > > + * If the function is successful, the only way to properly clean up the > > > > > + * memory is with a call to kobject_del(). > > > > > > > > In which case kobject_put() isn't needed? > > > > > > kobject_del() should only undo what kobject_add() did. So kobject_put() > > > will still be needed to clean up the memory. Perhaps the wording should > > > be: > > > > > > If the function is successful, the only way to properly clean up the > > > kobject is to call kobject_del() for removing the kobject from the > > > hierarchy and to subsequently call kobject_put() to clean up the memory. > > > > But that's not what the code does today in the kobject_del() function. > > Hm, if I'm not completely confused, kobject_del() just gives up the > extra reference obtained by kobject_add(). That leaves the initial > reference we got via kobject_init(_ng)(). If we want to clean up the > memory, we need to give up that reference as well. That's right. Furthermore there are other parts of the kernel that expect to do the kobject_del() and the final kobject_put() separately. For example, the driver core does kobject_del() as part of device_del() and then it does the final kobject_put() as part of put_device(). Alan Stern ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions 2007-12-01 3:29 ` [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions Alan Stern 2007-12-03 12:00 ` Cornelia Huck @ 2007-12-03 19:05 ` Greg KH 2007-12-03 20:55 ` Alan Stern 1 sibling, 1 reply; 10+ messages in thread From: Greg KH @ 2007-12-03 19:05 UTC (permalink / raw) To: Alan Stern Cc: Cornelia Huck, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Fri, Nov 30, 2007 at 10:29:39PM -0500, Alan Stern wrote: > On Fri, 30 Nov 2007, Greg KH wrote: > > > /** > > + * 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. > > + */ > > +void kobject_init_ng(struct kobject *kobj, struct kobj_type *ktype) > > Kerneldoc needs to be updated -- no @parent or @fmt. Also no error > returns. But you could say that after this routine runs, the kobject > should be deallocated by kobject_put() and not by calling kfree() > directly. Thanks, have now updated the comments. > > +/** > > + * kobject_add_ng - the main kobject add function > > + * @kobj: the kobject to add > > + * @parent: pointer to the parent of the kobject. > > + * > > + * The kobject name is set and added to the kobject hierarchy in this > > + * function. > > + * > > + * If @parent is set, then the parent of the @kobj will be set to it. > > + * If @parent is NULL, then the parent of the @kobj will be set to the > > + * kobject associted with the kset assigned to this kobject. If no kset > > + * is assigned to the kobject, then the kobject will be located in the > > + * root of the sysfs tree. > > + * > > + * If this function returns an error, kobject_put() must be called to > > + * properly clean up the memory associated with the object. > > + * > > + * If the function is successful, the only way to properly clean up the > > + * memory is with a call to kobject_del(). > > In which case kobject_put() isn't needed? Yes, now documented. > > + * > > + * Under no instance should the kobject that is passed to this function > > + * be directly freed with a call to kfree(), that can leak memory. > > + */ > > Should you say something here about uevents? Can't hurt, I added a sentance to it. > > +int kobject_add_ng(struct kobject *kobj, struct kobject *parent, > > + const char *fmt, ...) > > +{ > > + va_list args; > > + int retval; > > + > > + if (!kobj) > > + return -EINVAL; > > + > > + va_start(args, fmt); > > + retval = kobject_set_name_vargs(kobj, fmt, args); > > + va_end(args); > > + if (retval) { > > + printk(KERN_ERR "kobject: can not set name properly!\n"); > > + return retval; > > + } > > + kobj->parent = parent; > > + return kobject_add(kobj); > > +} > > +EXPORT_SYMBOL(kobject_add_ng); > > Looks like this should call kobject_add_varg() instead of duplicating > its code. Oops, yes, I ment to do that, thanks for pointing out I missed it. Now fixed. > > +/** > > + * 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 kobject passed to this function > > + * must be cleaned up by calling kobject_put(), and not by directly > > + * trying to call kfree() on the kobject. > > + * > > + * If this function succeeds, the only way to properly clean up the > > + * kobject is to call kobject_destroy(), which will clean up all of the > > kobject_destroy()? Where did that come from? Or did you mean > kobject_del()? Yes, stupid me... > > + * needed sysfs objects, and the kobject itself (by calling back to the > > + * ktype->release() function.) > > + * > > + * Note that the kobject_uevent() call should be called after this > > + * function succeeds, so that userspace can properly know that the > > + * kobject was created. > > + */ > > Could the comments be made shorter by saying merely that this routine > combines calls to kobject_init() and kobject_add_ng()? Good idea, now done. > > +int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype, > > + struct kobject *parent, const char *fmt, ...) > > +{ > > + va_list args; > > + int retval; > > + > > + kobject_init_ng(kobj, ktype); > > + > > + va_start(args, fmt); > > + retval = kobject_add_varg(kobj, parent, fmt, args); > > + va_end(args); > > + > > + return retval; > > +} > > +EXPORT_SYMBOL(kobject_init_and_add); > > Looks okay. > > Did you want to add an extra kobject_put() to the end of kobject_del()? > Or did you want to define a new kobject_destroy() that combines calls > to kobject_del() and kobject_put()? No, let's keep what we have there on the cleanup path for now. I have enough work to clean up the initializion part of the api for now :) Here's the updated patch, thanks for the review comments. thanks, greg k-h --------------- Date: Thu, 29 Nov 2007 22:38:12 -0800 To: Greg KH <greg@kroah.com> From: Greg Kroah-Hartman <gregkh@suse.de> Subject: kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions This is what the kobject_init and kobject_add functions are going to become. Add them to the kernel and then we can convert over the current kobject_init and kobject_add 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(); And we mark kobject_cleanup() as static, as no one outside of the kobject core uses it, nor should they ever. Cc: Kay Sievers <kay.sievers@vrfy.org> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> --- include/linux/kobject.h | 11 +++ lib/kobject.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 148 insertions(+), 7 deletions(-) --- a/include/linux/kobject.h +++ b/include/linux/kobject.h @@ -79,9 +79,16 @@ static inline const char * kobject_name( } extern void kobject_init(struct kobject *); -extern void kobject_cleanup(struct kobject *); - +extern void kobject_init_ng(struct kobject *kobj, struct kobj_type *ktype); extern int __must_check kobject_add(struct kobject *); +extern int __must_check kobject_add_ng(struct kobject *kobj, + 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_del(struct kobject *); extern struct kobject * __must_check kobject_create(const char *name, --- a/lib/kobject.c +++ b/lib/kobject.c @@ -290,6 +290,141 @@ int kobject_set_name(struct kobject *kob EXPORT_SYMBOL(kobject_set_name); /** + * kobject_init_ng - initialize a kobject structure + * @kobj: pointer to the kobject to initialize + * @ktype: pointer to the ktype for this kobject. + * + * This function will properly initialize a kobject such that it can then + * be passed to the kobject_add() call. + * + * After this function is called, the kobject MUST be cleaned up by a call + * to kobject_put(), not by a call to kfree directly to ensure that all of + * the memory is cleaned up properly. + */ +void kobject_init_ng(struct kobject *kobj, struct kobj_type *ktype) +{ + char *err_str; + + if (!kobj) { + err_str = "invalid kobject pointer!"; + goto error; + } + if (!ktype) { + err_str = "must have a ktype to be initialized properly!\n"; + goto error; + } + if (atomic_read(&kobj->kref.refcount)) { + /* do not error out as sometimes we can recover */ + printk(KERN_ERR "kobject: reference count is already set, " + "something is seriously wrong.\n"); + dump_stack(); + } + + kref_init(&kobj->kref); + INIT_LIST_HEAD(&kobj->entry); + kobj->ktype = ktype; + return; + +error: + printk(KERN_ERR "kobject: %s\n", err_str); + dump_stack(); +} +EXPORT_SYMBOL(kobject_init_ng); + +static int kobject_add_varg(struct kobject *kobj, struct kobject *parent, + const char *fmt, va_list vargs) +{ + va_list aq; + int retval; + + va_copy(aq, vargs); + retval = kobject_set_name_vargs(kobj, fmt, aq); + va_end(aq); + if (retval) { + printk(KERN_ERR "kobject: can not set name properly!\n"); + return retval; + } + kobj->parent = parent; + return kobject_add(kobj); +} + +/** + * kobject_add_ng - the main kobject add function + * @kobj: the kobject to add + * @parent: pointer to the parent of the kobject. + * @fmt: format to name the kobject with. + * + * The kobject name is set and added to the kobject hierarchy in this + * function. + * + * If @parent is set, then the parent of the @kobj will be set to it. + * If @parent is NULL, then the parent of the @kobj will be set to the + * kobject associted with the kset assigned to this kobject. If no kset + * is assigned to the kobject, then the kobject will be located in the + * root of the sysfs tree. + * + * If this function returns an error, kobject_put() must be called to + * properly clean up the memory associated with the object. + * + * If the function is successful, the only way to properly clean up the + * memory is with a call to kobject_del(), in which case, a call to + * kobject_put() is not necessary (kobject_del() does the final + * kobject_put() to call the release function in the ktype's release + * pointer.) + * + * Under no instance should the kobject that is passed to this function + * be directly freed with a call to kfree(), that can leak memory. + * + * Note, no uevent will be created with this call, the caller should set + * up all of the necessary sysfs files for the object and then call + * kobject_uevent() with the UEVENT_ADD parameter to ensure that + * userspace is properly notified of this kobject's creation. + */ +int kobject_add_ng(struct kobject *kobj, struct kobject *parent, + const char *fmt, ...) +{ + va_list args; + int retval; + + if (!kobj) + return -EINVAL; + + va_start(args, fmt); + retval = kobject_add_varg(kobj, parent, fmt, args); + va_end(args); + + return retval; +} +EXPORT_SYMBOL(kobject_add_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 combines the call to kobject_init_ng() and + * kobject_add_ng(). The same type of error handling after a call to + * kobject_add_ng() and kobject lifetime rules are the same here. + */ +int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype, + struct kobject *parent, const char *fmt, ...) +{ + va_list args; + int retval; + + kobject_init_ng(kobj, ktype); + + va_start(args, fmt); + retval = kobject_add_varg(kobj, parent, fmt, args); + va_end(args); + + return retval; +} +EXPORT_SYMBOL_GPL(kobject_init_and_add); + +/** * kobject_rename - change the name of an object * @kobj: object in question. * @new_name: object's new name @@ -444,12 +579,11 @@ struct kobject * kobject_get(struct kobj return kobj; } -/** - * kobject_cleanup - free kobject resources. - * @kobj: object. +/* + * kobject_cleanup - free kobject resources. + * @kobj: object. */ - -void kobject_cleanup(struct kobject * kobj) +static void kobject_cleanup(struct kobject *kobj) { struct kobj_type * t = get_ktype(kobj); struct kset * s = kobj->kset; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions 2007-12-03 19:05 ` Greg KH @ 2007-12-03 20:55 ` Alan Stern 0 siblings, 0 replies; 10+ messages in thread From: Alan Stern @ 2007-12-03 20:55 UTC (permalink / raw) To: Greg KH Cc: Cornelia Huck, Kay Sievers, Kernel development list, Jonathan Corbet, Randy Dunlap On Mon, 3 Dec 2007, Greg KH wrote: > > Did you want to add an extra kobject_put() to the end of kobject_del()? > > Or did you want to define a new kobject_destroy() that combines calls > > to kobject_del() and kobject_put()? > > No, let's keep what we have there on the cleanup path for now. I have > enough work to clean up the initializion part of the api for now :) > +/** > + * kobject_add_ng - the main kobject add function > + * @kobj: the kobject to add > + * @parent: pointer to the parent of the kobject. > + * @fmt: format to name the kobject with. > + * > + * The kobject name is set and added to the kobject hierarchy in this > + * function. > + * > + * If @parent is set, then the parent of the @kobj will be set to it. > + * If @parent is NULL, then the parent of the @kobj will be set to the > + * kobject associted with the kset assigned to this kobject. If no kset > + * is assigned to the kobject, then the kobject will be located in the > + * root of the sysfs tree. > + * > + * If this function returns an error, kobject_put() must be called to > + * properly clean up the memory associated with the object. > + * > + * If the function is successful, the only way to properly clean up the > + * memory is with a call to kobject_del(), in which case, a call to > + * kobject_put() is not necessary (kobject_del() does the final > + * kobject_put() to call the release function in the ktype's release > + * pointer.) This comment is inconsistent with your decision above not to add an extra kobject_put() into kobject_del(). (Unless such a line was added while I wasn't looking...) Alan Stern ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2007-12-04 15:28 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-12-01 1:00 [RFC] kobject_init changes - take 2 Greg KH 2007-12-01 1:01 ` [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions Greg KH 2007-12-01 1:02 ` [RFC] kobject: the new functions in use Greg KH 2007-12-01 3:29 ` [RFC] kobject: add kobject_init_ng, kobject_add_ng, and kobject_init_and_add functions Alan Stern 2007-12-03 12:00 ` Cornelia Huck 2007-12-03 19:06 ` Greg KH 2007-12-04 10:01 ` Cornelia Huck 2007-12-04 15:28 ` Alan Stern 2007-12-03 19:05 ` Greg KH 2007-12-03 20:55 ` Alan Stern
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).