Re: [mm Patch] isdn4linux: Gigaset driver: fix __must_check warning

[Andrew Morton <akpm@osdl.org>]

Tilman Schmidt <tilman@imap.cc> wrote:
>
> This patch to the Siemens Gigaset driver fixes the compile warning
> "ignoring return value of 'class_device_create_file', declared with
> attribute warn_unused_result" appearing with CONFIG_ENABLE_MUST_CHECK=y
> in release 2.6.18-rc1-mm1.
> 
> Signed-off-by: Tilman Schmidt <tilman@imap.cc>
> Acked-by: Hansjoerg Lipp <hjlipp@web.de>
> ---
> 
>  proc.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletion(-)
> 
> --- linux-2.6.18-rc1-orig/drivers/isdn/gigaset/proc.c	2006-07-09 17:19:49.000000000 +0200
> +++ linux-2.6.18-rc1-mm1-work/drivers/isdn/gigaset/proc.c	2006-07-09 18:31:15.000000000 +0200
> @@ -83,5 +83,6 @@ void gigaset_init_dev_sysfs(struct cards
>  		return;
>  
>  	gig_dbg(DEBUG_INIT, "setting up sysfs");
> -	class_device_create_file(cs->class, &class_device_attr_cidmode);
> +	if (class_device_create_file(cs->class, &class_device_attr_cidmode))
> +		dev_warn(cs->dev, "could not create sysfs attribute\n");
>  }

hm.

With this change we'll emit a warning (actually it's an error - I'll make
it dev_err(), OK?) and then we'll continue execution, pretending that the
sysfs file actually got registered.  Later, we'll try to unregister a
not-registered sysfs file.

So it's all a bit flakey when you look at it in a dumb fashion.

But I think the patch is OK - if that class_device_create_file() fails,
then there's some other bug somewhere, and the warning you've added is
sufficient - it tells the developers what the initial failure was, when it
happens.  So later, if someone reports a crash, we'll see that warning in
their logs and it'll lead us to the real bug.  We certainly couldn't
justify adding additional code which attempts to "continue working" if the
class_device_create_file() fails, because it just shouldn't fail.


It's probable that the message will never come out ever, so it's not worth
adding a ton of code to support this.

It'd be better if we had a class_device_create_file_warn() which does the
warning for you: its semantics are "this is expected to succeed".  But if
we do that to class_device_create_file() then we'd need to do it to 200
other things too.