Messages in this thread | | | Subject | Re: kernel/params.c: 'err' variable "set but not used" and perhaps should be? | From | Louis Langholtz <> | Date | Wed, 03 Jun 2015 13:41:40 -0600 |
| |
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.
| |