lkml.org 
[lkml]   [2007]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2007-07-12 21:49    [W:4.277 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site