Re: kernel/params.c: 'err' variable "set but not used" and perhaps should be?

Re: kernel/params.c: 'err' variable "set but not used" and perhaps should be?

[Louis Langholtz]

On Jun 1, 2015, at 7:32 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:

> Louis Langholtz <lou_langholtz@me.com> writes:
>> I get a compiler warning (on compiling the linux kernel) about the 'err'
>> variable being "set but not used" in the version_sysfs_builtin() function
>> of kernel/params.c (at line 848). Should it be used?
>> 
>> The 'err' variable is getting its value from the sysfs_create_file()
>> function which is marked '__must_check'. If it's used, a call at least to
>> printk() about any failure (indicated by a non-zero value) would seem
>> useful. Here's a patch to do just that (if that alone is helpful):
>> ...
> 
> That's hilarious.
> 
> __attribute__((warn_unused_result)) was added to gcc as a hack so people
> wouldn't forget to use the realloc return, which probably seemed sane.
> Explains why you can't suppress it by casting to void, because for
> realloc, that would be dumb.
> 
> Everyone loved it so much, they sprinkled little must-check turds
> everywhere!  Because MY FUNCTION IS IMPORTANT YOU SIMPLETON, YOU MUST
> CHECK THE RETURN!
> 
> The problem with yelling "YOU MUST DO SOMETHING" is that the answer is
> often "this is something, therefore it must be done".  That's what
> happened here.

The function sysfs_create_file is marked as __must_check in the
include/linux/sysfs.h file. This specific attribution appears to have been
added to this function back on September 20, 2007 by Tejun Heo. I have CC'd
Tejun so he has opportunity to respond to this criticism that you have raised.
Andrew Morton may have established the precedent for using
__must_check in this file with his August 14, 2006 commit with the message
that includes the following statements:
  "There's just no reason to ignore errors which can and do occur.  So the
   patch sprinkles __must_check all over these APIs."
Given this, I'd also like to hear what Andrew's thoughts are on this criticism.

> ...
> Instead, I suggest we introduce the following, taken literally from
> various bits of userspace code I've written:
> 
>        +/* Gcc's warn_unused_result is fascist bullshit. */
>        +#define doesnt_matter()
>        +#define doesnt_happen()
> 
> And apply it:
> 
> diff --git a/kernel/params.c b/kernel/params.c
> index a22d6a7..fafd94a 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -853,7 +853,8 @@ 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);
> +                       if (sysfs_create_file(&mk->kobj, &vattr->mattr.attr))
> +                               doesnt_happen();
>                        kobject_uevent(&mk->kobj, KOBJ_ADD);
>                        kobject_put(&mk->kobj);
>                }

Arguably then, the BUG_ON macro seems more appropriate for this situation
than this suggested doesnt_happen macro or my original offering of a call to
pr_warning.

I'm curious what the LKML thinks about this issue too.

Re: kernel/params.c: 'err' variable "set but not used" and perhaps should be?

[Tejun Heo]

Hello,

On Wed, Jun 03, 2015 at 01:41:40PM -0600, Louis Langholtz wrote:
> On Jun 1, 2015, at 7:32 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > That's hilarious.
> > 
> > __attribute__((warn_unused_result)) was added to gcc as a hack so people
> > wouldn't forget to use the realloc return, which probably seemed sane.
> > Explains why you can't suppress it by casting to void, because for
> > realloc, that would be dumb.
> > 
> > Everyone loved it so much, they sprinkled little must-check turds
> > everywhere!  Because MY FUNCTION IS IMPORTANT YOU SIMPLETON, YOU MUST
> > CHECK THE RETURN!
> > 
> > The problem with yelling "YOU MUST DO SOMETHING" is that the answer is
> > often "this is something, therefore it must be done".  That's what
> > happened here.
> 
> The function sysfs_create_file is marked as __must_check in the
> include/linux/sysfs.h file. This specific attribution appears to have been
> added to this function back on September 20, 2007 by Tejun Heo. I have CC'd
> Tejun so he has opportunity to respond to this criticism that you have raised.

Well, it's a while ago and I don't strongly object to removing it but
I still think it's a good idea to keep it around.  The failure of the
function creates a userland visible behavior difference and it's kinda
easy to forget that it may fail as it often is the final action taken
on the object.

> > @@ -853,7 +853,8 @@ 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);
> > +                       if (sysfs_create_file(&mk->kobj, &vattr->mattr.attr))
> > +                               doesnt_happen();
> >                        kobject_uevent(&mk->kobj, KOBJ_ADD);
> >                        kobject_put(&mk->kobj);
> >                }
> 
> Arguably then, the BUG_ON macro seems more appropriate for this situation
> than this suggested doesnt_happen macro or my original offering of a call to
> pr_warning.

It does happen.  If you don't wanna roll back on failure, just wrap it
in WARN_ON() so that there's at least some indication that something
failed there?  It'd kinda suck to be missing some interface files w/o
any indication, wouldn't it?

Thanks.

-- 
tejun

Re: kernel/params.c: 'err' variable "set but not used" and perhaps should be?

[Rusty Russell]

Tejun Heo <htejun@gmail.com> writes:
>> > @@ -853,7 +853,8 @@ 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);
>> > +                       if (sysfs_create_file(&mk->kobj, &vattr->mattr.attr))
>> > +                               doesnt_happen();
>> >                        kobject_uevent(&mk->kobj, KOBJ_ADD);
>> >                        kobject_put(&mk->kobj);
>> >                }
>> 
>> Arguably then, the BUG_ON macro seems more appropriate for this situation
>> than this suggested doesnt_happen macro or my original offering of a call to
>> pr_warning.
>
> It does happen.  If you don't wanna roll back on failure, just wrap it
> in WARN_ON() so that there's at least some indication that something
> failed there?  It'd kinda suck to be missing some interface files w/o
> any indication, wouldn't it?

Please describe the circumstances under which this function can fail.

Thanks,
Rusty.

Re: kernel/params.c: 'err' variable "set but not used" and perhaps should be?

[Tejun Heo]

On Thu, Jun 04, 2015 at 11:03:16AM +0930, Rusty Russell wrote:
> Please describe the circumstances under which this function can fail.

Allocation failure obviously and violatin of certain API rules -
e.g. dup names, wrong nesting, activation rule violations.  Some can
be warned automatically but I'm not sure about e.g. activation rule
violation.

Also, please note that w/ kmemcg order-0 allocation failures are a lot
more common and we probably won't want to print out warnings
automatically.  For module's use case, it can just trigger warnings
and continue on but I don't think it'd be a good idea to cripple the
API by making it trigger warnings internally and then make it return
void.

Thanks.

-- 
tejun

Re: kernel/params.c: 'err' variable "set but not used" and perhaps should be?

[Rusty Russell]

Tejun Heo <htejun@gmail.com> writes:
> On Thu, Jun 04, 2015 at 11:03:16AM +0930, Rusty Russell wrote:
>> Please describe the circumstances under which this function can fail.
>
> Allocation failure obviously

Won't happen here, this is a boot-time function.  version_sysfs_builtin.
The __init is the clue.

> and violatin of certain API rules -
> e.g. dup names, wrong nesting, activation rule violations.

Duplicated names imply some weird build error, and we get an warning in
that case already.

Not sure how we'd get wrong nesting or whatever activation rule
violations are, but happy to be enlightened?

Neither of the others justify version_sysfs_builtin checking the
return value of sysfs_create_file().

Thanks,
Rusty.

Re: kernel/params.c: 'err' variable "set but not used" and perhaps should be?

[Tejun Heo]

On Fri, Jun 05, 2015 at 05:16:53AM +0930, Rusty Russell wrote:
> Tejun Heo <htejun@gmail.com> writes:
> > On Thu, Jun 04, 2015 at 11:03:16AM +0930, Rusty Russell wrote:
> >> Please describe the circumstances under which this function can fail.
> >
> > Allocation failure obviously
> 
> Won't happen here, this is a boot-time function.  version_sysfs_builtin.
> The __init is the clue.

Yes, that's this one callsite.  There are whole others which can fail.
Just add WARN_ON here.  What are you arguing?

Thanks.

-- 
tejun

Re: kernel/params.c: 'err' variable "set but not used" and perhaps should be?

[Rusty Russell]

Tejun Heo <htejun@gmail.com> writes:
> On Fri, Jun 05, 2015 at 05:16:53AM +0930, Rusty Russell wrote:
>> Tejun Heo <htejun@gmail.com> writes:
>> > On Thu, Jun 04, 2015 at 11:03:16AM +0930, Rusty Russell wrote:
>> >> Please describe the circumstances under which this function can fail.
>> >
>> > Allocation failure obviously
>> 
>> Won't happen here, this is a boot-time function.  version_sysfs_builtin.
>> The __init is the clue.
>
> Yes, that's this one callsite.  There are whole others which can fail.
> Just add WARN_ON here.  What are you arguing?

What will a second warning which is never triggered achieve?  A bit of
code bloat and confusion, when I really do want to ignore the value.

I've asked (again) for gcc to allow cast to void:

        https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425

Thanks,
Rusty.

Re: kernel/params.c: 'err' variable "set but not used" and perhaps should be?

[Tejun Heo]

Hello, Rusty.

On Fri, Jun 05, 2015 at 10:09:29AM +0930, Rusty Russell wrote:
> What will a second warning which is never triggered achieve?  A bit of

How do you know that tho?  Somebody may change something in the module
code, kernfs or sysfs and break something in an unexpected way.  We've
always used BUG_ON() in __init functions to annotate things which
shouldn't fail.

> code bloat and confusion, when I really do want to ignore the value.

BUG_ON()s are very light weight, __init code gets dropped once done,
and this is an established way of annotating operations which aren't
expected to fail.

I'm having a hard time understanding the point of this thread.  :(

Thanks.

-- 
tejun