Messages in this thread | | | Date | Sat, 14 Jul 2007 09:57:17 +0530 | From | "Satyam Sharma" <> | Subject | Re: [PATCH 24/61] sysfs: make sysfs_put() ignore NULL sd |
| |
Hi Tejun,
On 7/14/07, Tejun Heo <htejun@gmail.com> wrote: > Hello, > > Satyam Sharma wrote: > > The whole _purpose_ of get()/put() functions (i.e. refcounting in general) > > is to ensure that the (shared) objects don't go away from under us while > > we're holding them. The proposed change _weakens_ the API itself by > > allowing a buggy driver (that somehow called into _put() codepath without > > a _get() before it) to not get flagged immediately (through an oops). This > > inevitably leads to some difficult-to-debug problems -- I have suffered > > debugging issues created by such weak/loose APIs myself. > > Yeah, that's the advantage of not allowing NULL, well, or disadvantage > of allowing. I get that. If I were to reimplement all these functions, > I probably wouldn't allow NULL argument myself either.
Good, at least we're on the same plane here :-)
> As much as I > understand your POV, I just don't think it's as critical as maintaining > uniformity. > [...] if you make things confusing by allowing on some but not > allowing on others, it's much more likely to trigger programmer error. > [...] The advantages or disadvantages are really small > compared to the confusion overhead.
Well, I don't really buy the uniformity/confusion argument either:
fput() will oops on NULL file * put_super() will oops on NULL super_block * put_task_struct() will oops on NULL task_struct * mmput() will oops on NULL mm_struct * configfs_put() will oops on NULL configfs_dirent * kref_put() will oops on NULL kref * (and this one better do) dev_put() will oops on NULL net_device * put_driver() will oops on NULL device_driver *
and sysfs_put() also oopsed on NULL sysfs_dirent *, till you changed it not to, that is :-)
[ and there are other examples, of course ]
I think put()'s *must* oops on NULL arguments -- it's simply a matter of writing a good, strict, (I could add "tasteful" but that would make it subjective) API that will *prevent* mysterious bugs.
Of course, dput(), kobject_put() and put_device() are a few counter examples that allowing put-ing NULL objects, but it's not as if this _disease_ has become widespread enough throughout the kernel -- that is, till we _make_ it widespread by following the lead of *mistakes*.
[ Perhaps you don't really care for the examples I mentioned above because sysfs is "closer" in some sense to kobjects / devices and so because those allow put-ing NULL objects, you want to make sysfs "uniform" to them in that sense ... but that's a bad excuse. ]
> > I'd be gladder if you could point me to code where this change really > > helps. > > I'd definitely like to send patches, why not. > > It's added by later patches. The patch itself is created because I was > hit by the confusion. sysfs_get() allowed NULL argument but sysfs_put() > didn't. Where's sense in that? I thought about converting sysfs_get() > to not accepting NULL but then the whole kobject and friends would act > differently. > > For sysfs, changing behavior is okay. It's mostly self-contained > subsystem and sysfs_get/put() aren't even exported to outside world, but > kobject and driver model are different story. Changing such subtle > behavior of such widely used interface is bound to create a lot of > confusion.
Well, what I'm asking here is not to change the mistakes already made (it's probably too late for that), but to avoid *repeating* them.
> Think about what would happen if NULL is suddenly disallowed on kfree(). > Even if all the current in-kernel users are converted at once (I'm sure > we're gonna miss some tho), all the out of tree (including devel) codes > and future codes will suffer and some of the bugs would be really hard > to catch as kfree(NULL) is buried in error handling path which generally > isn't triggered too often.
kfree(NULL) is not exactly analogous. And obviously I don't intend changing that and then trying to change the 6,732,893 or so callsites that assume the same :-) But what if everybody starts taking this lead and starts {re}writing APIs accordingly throughout the kernel ... *shudder to think*
Anyway, we've wasted enough time and bandwidth discussing this (relatively trivial) matter, and I know nobody's mind is changed after the end of it all (at least mine won't), so I suggest let's stop. The proposed change is in Greg's tree already, and if he's fine with it, then there's not much to do about it, is there :-)
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/
| |