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@xxxxxxxxxxxxxxx> wrote:

> Louis Langholtz <lou_langholtz@xxxxxx> 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.--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/