Content-type: text/plain; charset=us-ascii
MIME-version: 1.0 (Mac OS X Mail 7.3 \(1878.6\))
Subject: Re: [PATCH] kernel/params.c: make use of unused but set variable
From: Louis Langholtz <lou_langholtz@me.com>
In-reply-to: <20150608005848.GA21465@mtj.duckdns.org>
Date: Wed, 10 Jun 2015 11:05:21 -0600
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        trivial@kernel.org, Rusty Russell <rusty@rustcorp.com.au>,
        Andrew Morton <akpm@linux-foundation.org>
Content-transfer-encoding: 8BIT
Message-id: <1A6DE1E8-4E0E-4894-A07F-A1EE9AC55835@me.com>
References: <1433721270-9182-1-git-send-email-lou_langholtz@me.com>
 <20150608000007.GA3543@mtj.duckdns.org>
 <CA+55aFwmOGaP1DN3frkfP+Df=jCwURqO22A6-5GqpMnZXkvMXA@mail.gmail.com>
 <20150608005848.GA21465@mtj.duckdns.org>
To: Tejun Heo <htejun@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3539
Lines: 76

On Jun 7, 2015, at 6:58 PM, Tejun Heo <htejun@gmail.com> wrote:

> On Sun, Jun 07, 2015 at 05:17:20PM -0700, Linus Torvalds wrote:
>> At most, it could be a "WARN_ON_ONCE()". Maybe even just silently
>> ignore the error. But BUG_ON()? Hell no.
> 
> Yeah, WARN_ON_ONCE() is the right one...
> 
> -- 
> tejun

On further consideration...

While removing the __must_check attribute from the sysfs_create_file
function seems to have larger ramifications (as it's called more often than
the code I'd offered a patch for), is it really that helpful to keep this
attribution?

I don't believe so now and think it should be removed. Here's why (stuff
that's partly been said already):

The underlying code for sysfs_create_file does call WARN to warn about
any errors. So it's not like the code is totally silent anyway. Then the unused
but set variable in params.c (that's set to its return value) can be removed
(and the compiler warning resolved). Incidentally, I do think it'd be helpful
to comment document this behavior of using WARN so that callers can
more readily recognize (and be assured that) they won't need to call WARN
(unless callers want to add __FILE__ and __LINE__ info to the printk output).

Having functions marked __must_check seems to make more sense when
their return values are *always* necessary for calling code to have any
business calling them. Like when there's one or more future calls to functions
that have to be made (or not made) for the given resource (the kobject) that
depend on that function's return value (such that the return value state is
always a later conditional). That doesn't appear to be the case however for
sysfs_create_file().

Seems what Rusty was indicating too.

We never did hear back from Andrew though who had added many (most?)
of the __must_check attributes to begin with to the sysfs interface functions
(in include/linux/sysfs.h). The full commit message back in 2006 stated:

    add __must_check to device management code
    
    We're getting a lot of crashes in the sysfs/kobject/device/bus/class code and
    they're very hard to diagnose.
    
    I'm suspecting that in some cases this is because drivers aren't checking
    return values and aren't handling errors correctly.  So the code blithely
    blunders on and crashes later in very obscure ways.
    
    There's just no reason to ignore errors which can and do occur.  So the patch
    sprinkles __must_check all over these APIs.
    
    Causes 1,513 new warnings.  Heh.

Adding __must_check probably made it easier for developers to identify calling
code that was depending on success from these functions. At the same time,
it's not true (at least currently) that these return values always need to
be checked to avoid incorrect behavior (though perhaps they should almost
always be checked).

Take the sysfs_chmod_file() function as another example of a function whose
return value is unnecessary to always check. It doesn't have the same
behavior though as sysfs_create_file() in internally calling WARN for any error
condition it might return. So not checking it may result in silent failures (hate
those), but still many of its callers don't do anything with its return value other
than warn about errors anyway (and don't seem like they need to do more).

--
Lou--
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/