Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1160187 > unrolled thread

Re: [PATCH] kernel/params.c: make use of unused but set variable

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] kernel/params.c: make use of unused but set variable Tejun Heo <htejun@gmail.com> - 2015-06-08 02:10 +0200
    Re: [PATCH] kernel/params.c: make use of unused but set variable Linus Torvalds <torvalds@linux-foundation.org> - 2015-06-08 02:20 +0200
      Re: [PATCH] kernel/params.c: make use of unused but set variable Tejun Heo <htejun@gmail.com> - 2015-06-08 03:00 +0200
        Re: [PATCH] kernel/params.c: make use of unused but set variable Tejun Heo <htejun@gmail.com> - 2015-06-11 04:00 +0200
          Re: [PATCH] kernel/params.c: make use of unused but set variable Louis Langholtz <lou_langholtz@me.com> - 2015-06-12 05:20 +0200
      Re: [PATCH] debug: Deprecate BUG_ON() use in new code, introduce  CRASH_ON() Ingo Molnar <mingo@kernel.org> - 2015-06-08 13:30 +0200
      Re: [PATCH] debug: Deprecate BUG_ON() use in new code, introduce  CRASH_ON() Ingo Molnar <mingo@kernel.org> - 2015-06-08 13:30 +0200
        Re: [PATCH] debug: Deprecate BUG_ON() use in new code, introduce  CRASH_ON() Ingo Molnar <mingo@kernel.org> - 2015-06-08 21:40 +0200
      Re: [PATCH] kernel/params.c: make use of unused but set variable Tejun Heo <htejun@gmail.com> - 2015-06-12 03:50 +0200
        Re: [PATCH] kernel/params.c: make use of unused but set variable Tejun Heo <htejun@gmail.com> - 2015-06-16 22:00 +0200
      Re: [PATCH] kernel/params.c: make use of unused but set variable Rusty Russell <rusty@rustcorp.com.au> - 2015-06-12 03:50 +0200

#1160187 — Re: [PATCH] kernel/params.c: make use of unused but set variable

On Sun, Jun 07, 2015 at 05:54:30PM -0600, Louis Langholtz wrote:
> While Rusty Russell wants the return value of sysfs_create_file
> ignored, it's annotated '__must_check'. Tejun Heo made the annotaion
> and suggests just using BUG_ON(). Meanwhile the compiler warns that
> the 'err' variable is set but unused. This patch uses Tejun's
> suggestion. This eliminates the warning, satisfies the required check,
> and fails-fast with notice if sysfs_create_file actually ever fails
> (something that Rusty says should never happen when this code runs).
> 
> Signed-off-by: Louis Langholtz <lou_langholtz@me.com>
> ---
> 
> diff --git a/kernel/params.c b/kernel/params.c
> index a22d6a7..b04a752 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -853,6 +853,7 @@ static void __init version_sysfs_builtin(void)
>  		mk = locate_module_kobject(vattr->module_name);
>  		if (mk) {
>  			err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
> +			BUG_ON(err);

Maybe BUG_ON(sysfs_create_file(...)); is simpler?  Other than that,

 Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

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

[toc] | [next] | [standalone]

#1160188

On Sun, Jun 7, 2015 at 5:00 PM, Tejun Heo <htejun@gmail.com> wrote:
> On Sun, Jun 07, 2015 at 05:54:30PM -0600, Louis Langholtz wrote:
>> @@ -853,6 +853,7 @@ static void __init version_sysfs_builtin(void)
>>               mk = locate_module_kobject(vattr->module_name);
>>               if (mk) {
>>                       err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
>> +                     BUG_ON(err);
>
> Maybe BUG_ON(sysfs_create_file(...)); is simpler?  Other than that,

Hell no.

Stop with the random BUG_ON() additions.

I have said this before, and apparently I need to sat this again, and
probably I will have to say it in the future.

We don't add BUG_ON's for random reasons.

The *ONLY* acceptable reason for a BUG_ON() is if the machine is dead
anyway because of some major internal corruption.

We have too many BUG_ON's. We've had people add BUG_ON's because "this
cannot happen", and then it turns out they were wrong, and they just
killed the machine.

Dammit, there's no reason to add a BUG_ON() here in the first place,
and the reason of "but but it's an unused error return": is f*cking
retarded.

Stop this idiocy. We don't write crap code just to satisfy some random
coding standard or shut up a compiler error.

At most, it could be a "WARN_ON_ONCE()". Maybe even just silently
ignore the error. But BUG_ON()? Hell no.

NO NO NO.

Quite frankly, if you want to add error handling, then dammit, add it
right. And no, BUG_ON() is _never_ proper error handling.

BUG_ON() is for things like "uhhuh, somebody is trying to free a page
that is already free". That is some serious internal corruption.

BUG_ON() is _not_ for "I'm not doing any error handling, so I'll
sprinkle random lines of BUG_ON() like fairy dust to make the compiler
happen".

Really.  I'm getting very tired indeed of people adding BUG_ON's like
that. Stop it.

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

[toc] | [prev] | [next] | [standalone]

#1160194

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.  The short history here is that
sysfs_create_file() has __must_check on it which triggered this whole
discussion.  The rationale for having __must_check on the function was
that the function's failure leads to userland visible behavior
difference and it's very inviting to skip error handling on the
function as file creation is often the last operation to be performed
with no further dependency on it.

Thanks.

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

[toc] | [prev] | [next] | [standalone]

#1162964

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.  The former is a pretty fundamental usage error and we've had
too many of the latter unhandled despite of __must_check.

> 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().

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?  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.

...
> 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.

I don't think removing __must_check would be a disaster 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.

Thanks.

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

[toc] | [prev] | [next] | [standalone]

#1163743

Hi Tejun,

On Jun 10, 2015, at 7:54 PM, Tejun Heo <htejun@gmail.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1160414 — Re: [PATCH] debug: Deprecate BUG_ON() use in new code, introduce CRASH_ON()

* Alexander Holler <holler@ahsoftware.de> wrote:

> > Also note that BUG_ON() will make data corruption _worse_ statistically. Why? 
> > Because most data corruptions are unlikely to be perfectly detected by a 
> > BUG_ON(), and the BUG_ON() delays the finding of the underlying bug, so the 
> > bug will hit more people before it's fixed for good.
> >
> > So even in the cases where you could argue that the system needs to stop, 
> > because we have evidence of data corruption, it's statistically the better 
> > approach to continue and get kernel log info back to developers.
> 
> Risking more, maybe even worse problems like corrupting file systems or similiar 
> in order to have a slightly chance of save log info?

That's not what I said - please read my argument and argue with that if you want, 
not with some other straw-man argument...

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1160415 — Re: [PATCH] debug: Deprecate BUG_ON() use in new code, introduce CRASH_ON()

* Alexander Holler <holler@ahsoftware.de> wrote:

> > I am pretty certain that Greg would have applied such a patch in an eye blink.
> 
> As you've said it, *probably*. But such a simple exit path as you're proposing 
> doesn't always exist. [...]

As I said it's case by case. I discussed your example (which was a deficient patch 
for multiple reasons) but we'd be wasting everyone's time by discussion 
hypothethical situations.

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1160734 — Re: [PATCH] debug: Deprecate BUG_ON() use in new code, introduce CRASH_ON()

* Alexander Holler <holler@ahsoftware.de> wrote:

> Am 08.06.2015 um 13:27 schrieb Ingo Molnar:
> >
> >* Alexander Holler <holler@ahsoftware.de> wrote:
> >
> >>>I am pretty certain that Greg would have applied such a patch in an eye blink.
> >>
> >>As you've said it, *probably*. But such a simple exit path as you're proposing
> >>doesn't always exist. [...]
> >
> >As I said it's case by case. I discussed your example (which was a deficient patch
> >for multiple reasons) but we'd be wasting everyone's time by discussion
> >hypothethical situations.
> 
> Sure it was a deficient patch, but still better than what existed for a year 
> long in the kernel [...]

... but it was still worse than the simple solution I suggested:

	if (WARN_ON_ONCE(port->itty))
		return;

again, the BUG_ON() you wanted to introduce was wrong on multiple grounds, then 
and now. Why are you still arguing about this?

> I just want to make clear that a brutforce slogan like BUG_ON is bad is bad,

I pointed out specific cases where a BUG_ON() is the right solution. They are 
rare. Stop misrepresenting my words.

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1163706

On Fri, Jun 12, 2015 at 10:57:24AM +0930, Rusty Russell wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> > On Sun, Jun 7, 2015 at 5:00 PM, Tejun Heo <htejun@gmail.com> wrote:
> > At most, it could be a "WARN_ON_ONCE()". Maybe even just silently
> > ignore the error. But BUG_ON()? Hell no.
> 
> Yeah, in practice it's already (1) paniced if we ran out of memory, or
> (2) warned if we somehow tried to create two entries with the same name.
> 
> So the WARN_ON_ONCE() is a bit... meh.  How's this, too snarky?

Sounds pretty passive agressive to me.  At least reply to the actual
argument?

Thanks.

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

[toc] | [prev] | [next] | [standalone]

#1166347

Hello, Rusty.

On Mon, Jun 15, 2015 at 05:19:26AM +0930, Rusty Russell wrote:
> Oh.  Perhaps my sense of humour is miscalibrated.

Heh, prolly mine was.  Sorry if I came off as aggressive.

> ===
> Subject: params: suppress unused variable error, warn once just in case code changes.
> 
> It shouldn't fail due to OOM (it's boot time), and already warns if we
> get two identical names.  But you never know what the future holds, and
> WARN_ON_ONCE() keeps gcc happy with minimal code.
> 
> Reported-by: Louis Langholtz <lou_langholtz@me.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> diff --git a/kernel/params.c b/kernel/params.c
> index 7edf31f2ce96..0b9bbdf830cb 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -884,6 +884,7 @@ static void __init version_sysfs_builtin(void)
>  		mk = locate_module_kobject(vattr->module_name);
>  		if (mk) {
>  			err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
> +			WARN_ON_ONCE(err);
>  			kobject_uevent(&mk->kobj, KOBJ_ADD);

Looks good to me.  Please feel free to add

 Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

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

[toc] | [prev] | [next] | [standalone]

#1163707

Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Sun, Jun 7, 2015 at 5:00 PM, Tejun Heo <htejun@gmail.com> wrote:
> At most, it could be a "WARN_ON_ONCE()". Maybe even just silently
> ignore the error. But BUG_ON()? Hell no.

Yeah, in practice it's already (1) paniced if we ran out of memory, or
(2) warned if we somehow tried to create two entries with the same name.

So the WARN_ON_ONCE() is a bit... meh.  How's this, too snarky?

Thanks,
Rusty.

Subject: params: suppress unused variable error, warn once just in case code changes.

Reported-by: Louis Langholtz <lou_langholtz@me.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/kernel/params.c b/kernel/params.c
index 7edf31f2ce96..0b9bbdf830cb 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -884,6 +884,12 @@ static void __init version_sysfs_builtin(void)
 		mk = locate_module_kobject(vattr->module_name);
 		if (mk) {
 			err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
+			/*
+			 * That should not fail at boot due to OOM, and it'll
+			 * already warn if we somehow get two identical names,
+			 * but this one line should quiet both gcc and lkml.
+			 */
+			WARN_ON_ONCE(err);
 			kobject_uevent(&mk->kobj, KOBJ_ADD);
 			kobject_put(&mk->kobj);
 		}
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web