Date:	Thu, 11 Jun 2015 21:17:32 -0600
From:	Louis Langholtz <lou_langholtz@...com>
To:	Tejun Heo <htejun@...il.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	trivial@...nel.org, Rusty Russell <rusty@...tcorp.com.au>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] kernel/params.c: make use of unused but set variable

Hi Tejun,

On Jun 10, 2015, at 7:54 PM, Tejun Heo <htejun@...il.com> wrote:

> Hey, Louis.
> 
> On Wed, Jun 10, 2015 at 11:05:21AM -0600, Louis Langholtz wrote:
>> 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
> 
> Not any errors.  It triggers warning on missing ops and dup file
> names.

Ah... I see now that the underlying code doesn't warn about all errors.
Specifically that code doesn't warn about __kernfs_create_file() returning
any error code other than EEXIST. So it's quiet on a few other error codes
it can return (like ENOMEM or ENOENT). Warnings are generated when
sysfs_create_file() returns EINVAL or EEXIST.

> The former is a pretty fundamental usage error and we've had
> too many of the latter unhandled despite of __must_check.

Admittedly the preconditions for sysfs_create_file() seem obscure to me.
Perhaps that's contributed to the problem you've described (some
comment documentation for the sysfs_create_file function I think
would be useful). And what should be done after this function returns?
Having this __must_check says its return value must be used. But how
should it be used? More on this question after next quote block...

> 
>> ...
>> 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().
> 
> I don't know.  Sounds like a weird rule.  We had __must_check, still
> had certain error types unhandled so people added explicit WARN to
> force people to look into those issues and that means __must_check
> should go away?

Taken in light of the sysfs_create_file function the rule seems (at least for
the moment) helpful in addressing what should be done about the return
value of sysfs_create_file().

Should the caller WARN about all errors returned from sysfs_create_file()?
Even those already reported (like when sysfs_create_file() returns EINVAL
or EEXIST)? If all errors should be WARNed (by code somewhere) then
would it be better to have the code underlying sysfs_create_file() do that?
If the code underlying sysfs_create_file() warns about all errors, then no
caller needs to duplicate that code. Alternatively the caller could just warn
when the return code isn't one that's already be warned about. But as the
code is now, that relies on how sysfs_create_file() underlying code is coded
to know what's already been warned and breaks as soon as someone
changes what sysfs_create_file() warns about without updating what the
callers warn about. That solution seems unnecessarily fragile.

What else should always be done besides provide WARN for errors? If
nothing else should always be done and errors are logged by the
underlying code, then what point is left for sysfs_create_file() being
marked __must_check? It shouldn't be - at least by this offered rule -
since its return value isn't always necessary for the caller to still have
business in calling it given that it has a worthwhile side-effect (when it
succeeds).

> Sure, if it warns on all errors and can return void
> then there's nothing to discuss but that's not the case.  Here, an
> error return indicates userland visible behavior difference.  I'd
> venture to say that the return value is pretty darn important.

I don't mean to diminish the significance of userland visible behavior
difference. These are significant. And I don't want sysfs_create_file()
to return void. But if there's no clear things that callers of sysfs_create_file()
should do based on its return value then marking it __must_check seems
wrong. I'm guessing (based on the commit log message) that Andrew
originally established the precedent for this marking though to help get
rid of cases where callers needed to do something different than they
were when errors occurred because their code was blindly assuming
success.

The return value of kmalloc I'd argue meanwhile should always be used.
Oddly it's not marked __must_check. But based on this rule, it makes more
sense to. What business would a caller have in calling kmalloc if it's not
going to use the return value? There is a side-effect of using up memory
but I don't know of a reason that's ever desirable for a properly running
kernel if the memory is not used (or at least passed to the free function).
Fortunately I don't see any callers that aren't using the return value and
marking it __must_check only helps to reaffirm what callers already know.

> ...
>> 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
> 
> I don't know.  The "always" rule you're speaking of seems too
> arbitrary and rigid to me.  Always is a tricky word and tying oneself
> to things like that usually doesn't lead to healthy trade-offs.

An understandable concern. I'd not want a rule to be established if
it can have holes poked into it. But __must_check is itself an always
kind of rule at least in so far as the compiler will always warn on the
result being unused (assuming it recognizes the attribute and it hasn't
been disabled). So having an always kind of rule for when
__must_check should or should not be used seems consistent with
what it's being applied to and seems like it'd be great to have if it's solid.

> I don't think removing __must_check would be a disaster

Sounds like what Rusty suggested too. Though I'm unclear if he preferred
removing this __must_check attribute or hacking around it with his suggested
doesnt_matter() macro. Hopefully it's the former.

> but at the
> same time these terminal sysfs functions look like the perfect
> candidates for such annotations where error returns indicate subtle
> but visible behavior differences visible to userland while not
> affecting in-kernel operations at all and thus can be easily ignored.
> ...

Disagreeing with this seems to put me at odds with Andrew Morton too.
Some of the sysfs functions may be proper candidates for this attribute
but not sysfs_create_file() as I've argued. Hopefully my not following in
these shoes will keep me out of trouble better than my following the
BUG_ON shoes (by offering the patch that used BUG_ON) got me into
trouble!  ;)

> Thanks.
> 
> -- 
> tejun

And my thanks to you too.

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

Powered by blists - more mailing lists