[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-id: <7E8FFE0D-B9F7-4816-8ECF-2AA5980F3890@me.com>
Date: Wed, 03 Jun 2015 13:41:40 -0600
From: Louis Langholtz <lou_langholtz@...com>
To: Rusty Russell <rusty@...tcorp.com.au>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org, htejun@...il.com
Subject: Re: kernel/params.c: 'err' variable "set but not used" and perhaps
should be?
On Jun 1, 2015, at 7:32 PM, Rusty Russell <rusty@...tcorp.com.au> wrote:
> Louis Langholtz <lou_langholtz@...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.--
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