Messages in this thread | | | Date | Fri, 13 Jul 2007 01:16:35 +0530 | From | "Satyam Sharma" <> | Subject | Re: [PATCH 24/61] sysfs: make sysfs_put() ignore NULL sd |
| |
Hi,
> >>>> Make sysfs_put() ignore NULL sd instead of oopsing. > >>> I do not think this is a good idea; it is non-sense (and rather a bug) > >>> to call "put" with NULL argument in general. > >> It's better than having to check it all the time in the caller :) > > > > How many callers do we have that will get benefit from this change? > > > > Well, the change will hide the bug. It seems all callers in fs/sysfs > > already assume that the argument is NOT NULL, and it is a bug to call > > sysfs_put() with NULL; the function should be used to "put" something > > you "have" (non-NULL). If it is called with NULL, I would say, we > > should BUG here to detect the logical bug.
Yoshifuji is 100% correct, IMNSHO.
Please, this is _basic_ refcounting semantics. For those who disagree, kindly read Yoshifuji's above paragraph again.
> Well, I'm okay either way. It's not like one way is undisputably better > than the other
Yes, it is, of course. Allowing xxx_put(NULL) to succeed (without any warnings/oops) is *absolutely* nonsensical, and can *only* occur if the caller (or worse, the API itself) is buggy in the first place (i.e. does not use proper locking and/or refcounting).
I can't believe it should be so difficult to understand this. How can any caller (that first did a xxx_get() on that shared object) land up with that object getting NULL _from under it_ unless some logic is wrong somewhere? And instead of flagging this broken logic, the proposed change here would hide it.
Worse, if that object did become NULL between the _get() and _put() code, then we'll have an oops (which would be even more difficult to debug now) anyway.
> but we're leaning toward accepting NULL argument in this > type of functions. Think about kfree(NULL) and its usefulness.
Don't {mis}quote the kfree() mistake here, please.
> More > importantly, the ecosystem around sysfs - that is, kobject, driver model > - generally accepts NULL argument for their get/put functions
This can only mean two things:
(1) Either, they simply do not _need_ the refcounting in the first place (which means -- better do away with get() and put() for them altogether)
(2) Or, all that code / APIs are so horribly misdesigned and/or buggy that you're now having to hide that by allowing NULL arguments in get() and put() functions (which means -- fix the bugs, please)
> so unless > there's a compelling reason to convert them all, and I don't see any, > sysfs_put() needs to follow the same rule.
Ok, what's the compelling reason to change sysfs_put() then? I don't see any, either.
Satyam - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |