Messages in this thread | | | Date | Tue, 25 May 2004 02:08:03 +0400 | From | Andrew Zabolotny <> | Subject | Re: class_device_find() |
| |
On Mon, 24 May 2004 09:46:31 -0700 Greg KH <greg@kroah.com> wrote:
> > The class_device_find() function returns a pointer to a class_device, but > > the reference counter of that object is not incremented. This looks to me > > somewhat racy, unless there are some details I'm missing. > There were lots of problems with that function, and that's one reason > it's not in the kernel tree :) Oops, it looks like I was looking at the wrong patch :-( My comments were true for original Jamey's patches, but your patch already has all these issues fixed. Mea culpa.
> > Because in the first case if class_device_get succeeds, but class_id is > > empty, the kobject will remain in a referenced state. > Good catch. But your fix is not quite correct, we should call > class_device_put() on the class_dev variable before returning -EINVAL > instead. That was obvious, but it's more code and no extra functionality. The same erroneous conditions will be caught, just a bit earlier, so there's no need to call put() since we caught it before we called get(). Unless there's no other condition when get() returns NULL, we are safe to call get() and do not check for the return value.
> > And yet one more comment. What is the purpose of class_device_get at the > > beginning and class_device_put at the end of the above function? > We are just being safe. IMHO it is useless to try to be safe for the reasons I pointed out. If somebody calls class_rename() without holding a reference, there's anyway a race condition. And if it calls class_rename holding a reference, there's no need to call get/put. Doing get/put will just minimize the chance of a race, not remove it.
Ok, apart from this discussion, what you don't like about the class_device_find() function? I mean your fixed implementation. The locks are in place, the returned object has got his reference counter increased. Or, alternatively, any other ideas how we can solve the problem I've described in my previous message?
Also one more question. Do you think if it is okay if a subclass (such as the lcd device class) does strcpy() directly to class_device->class_id, or it is worth to add a function class_dev_set_name(struct class_device *, const char*)?
I've attached your version of class_device_find() yet again, with a tiny fix (changed name arg to "const char *" in rename() and find(), and exported rename() which looks like it has been forgotten to be). Also I still would remove the get()/put() from rename() but this has not been done in this patch.
-- Greetings, Andrew [unhandled content-type:application/octet-stream] | |