lkml.org 
[lkml]   [2004]   [May]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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]
\
 
 \ /
  Last update: 2005-03-22 14:03    [W:1.265 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site