kobject_init changes

lib/kobject.c +18 -0

+/**
+ * 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;

Quoted 18 lines · Greg KH

lib/kobject.c +18 -0

+/**
+ * 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;

lib/kobject.c int kobject_set_name(struct kobject *kob · line 435

Quoted 10 lines

lib/kobject.c +10 -0

+
+	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().

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 6 lines · Greg KH

Quoted 5 lines

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().

Quoted 2 lines

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?

Quoted 9 lines · Greg KH

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.)

Quoted 5 lines

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.

Quoted 4 lines

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?

Quoted 3 lines

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.

Quoted 4 lines · Greg KH

And you
can't know that, so you have to call kobject_put() in order to be safe and clean up everything.

Quoted 3 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 6 lines · Greg KH

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, ...);

Quoted 3 lines

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().