Re: [PATCH 24/61] sysfs: make sysfs_put() ignore NULL sd

[Satyam Sharma]

Hi,

Thanks for explaining the purpose behind the proposed change,
but I'm still not convinced that allowing NULL argument in _put()
is a good idea, however:

On 7/13/07, Tejun Heo <htejun@xxxxxxxxx> wrote:
> Satyam Sharma wrote:
> > Please, this is _basic_ refcounting semantics. For those who disagree,
> > kindly read Yoshifuji's above paragraph again.
>
> I did but I don't really see anything so basic about refcounting
> semantics there.
> [...]
> > Ok, what's the compelling reason to change sysfs_put() then?
> > I don't see any, either.
>
> Because mixed situation is undisputably worse than one way or the other
> && making sysfs_put() to conform to its surroundings is the shortest
> path to achieve uniformity.  Gees, what's so important about allowing or
> not allowing NULL?

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.

> >> 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.
>
> Like it or not, kfree(NULL) is used the same way.

But we pay a heavy price for the "programmer convenience" (just
avoiding some conditionals / multiple goto labels in error cleanup
paths) that we get in return. I do believe the kernel would've been
much less buggy, if only kfree(NULL) wasn't legal.

But even if you do want to allow NULL arguments to such functions,
for certain situations, you could always write a little wrapper over
the lower function that'll include the "if (not NULL)" kind of check.

> >> 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)
>
> I don't really see how you can jump there from allowing NULL argument.
> Your conclusion is really far from the origin.
> [...]
> NULL put is usually used to simplify error handling / cleanup codes.

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.

On 7/13/07, Tejun Heo <htejun@xxxxxxxxx> wrote:
> >> 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.
>
> I agree with you about get().  Allowing NULL argument doesn't really
> help anything.  It only increases the chance of getting things wrong.
> I'm all for not allowing NULL argument to get().  For put(), as I wrote
> before, I think allowing NULL has some advantages and I don't care
> either way as long as it's not confusing.  The 'not confusing' part is
> way more important to me than advantages of either way.

As I said, changing generic refcounting API functions (which is what
xxx_put() is) to simplify open-coding of a conditional in the caller's
error cleanup paths does not sound advantageous to me at all. On the
contrary, it weakens the refcounting API, and I can only imagine the
kernel getting buggier with such changes.

Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/