led_class: storing a value can act but return -EINVAL

led_class: storing a value can act but return -EINVAL

[Johannes Berg]

When I store something into the brightness sysfs attribute of an LED, it
will accept the value but return -EINVAL:

johannes:/sys/class/leds/pmu-front-led# echo 255 > brightness
bash: echo: write error: Invalid argument

(yet the LED turns on)

This happens because the store callback doesn't consume all the input.

There are two possible ways to handle this:
a) accept anything that begins with a valid number.
b) reject anything that isn't *only* a number

The following patch implements a), for b) you'd have to make the if
statement have ' && after-buf == size' instead of this patch.

I don't know which approach is generally preferred, but acting and then
returning an error value doesn't seem nice.
Maybe b) should be implemented instead to stop people from storing
things like '0 hahaha stupid kernel ignores this' into the attribute :)

johannes

--- linux-2.6.orig/drivers/leds/led-class.c	2006-04-29 13:23:49.013288994 +0200
+++ linux-2.6/drivers/leds/led-class.c	2006-04-29 13:28:14.183288994 +0200
@@ -45,7 +45,7 @@ static ssize_t led_brightness_store(stru
 	unsigned long state = simple_strtoul(buf, &after, 10);
 
 	if (after - buf > 0) {
-		ret = after - buf;
+		ret = size;
 		led_set_brightness(led_cdev, state);
 	}
 

Re: led_class: storing a value can act but return -EINVAL

[Pavel Machek]

Hi!

> When I store something into the brightness sysfs attribute of an LED, it
> will accept the value but return -EINVAL:
> 
> johannes:/sys/class/leds/pmu-front-led# echo 255 > brightness
> bash: echo: write error: Invalid argument
> 
> (yet the LED turns on)
> 
> This happens because the store callback doesn't consume all the input.

Well, I'd argue current behaviour is okay... can you strace it? It
should accept the number (return 3) then return -EINVAL.

> There are two possible ways to handle this:
> a) accept anything that begins with a valid number.
> b) reject anything that isn't *only* a number

c) accept anything that is number, ignore newlines.

a) is just way too ugly...
						Pavel

-- 
Thanks, Sharp!

Re: led_class: storing a value can act but return -EINVAL

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

Hi,

> Well, I'd argue current behaviour is okay... can you strace it? It
> should accept the number (return 3) then return -EINVAL.

That's exactly what happens.

Which is totally bogus, because userspace will think that the setting
didn't succeed. Or application authors will ignore the return value
assuming that it always succeeded. Or read the value back to see if it
succeeded. All icky, when we can well have a good return value.

> > There are two possible ways to handle this:
> > a) accept anything that begins with a valid number.
> > b) reject anything that isn't *only* a number
> 
> c) accept anything that is number, ignore newlines.

Which is kinda hard to implement.

> a) is just way too ugly...

Well, I'd argue that it doesn't matter much since sysfs values are by
definition a single value per file, so you'll already know that putting
multiple "values" in is bogus.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

Re: led_class: storing a value can act but return -EINVAL

[Richard Purdie]

On Sun, 2006-04-30 at 13:01 +0200, Johannes Berg wrote:
> Hi,
> 
> > Well, I'd argue current behaviour is okay... can you strace it? It
> > should accept the number (return 3) then return -EINVAL.
> 
> That's exactly what happens.
> 
> Which is totally bogus, because userspace will think that the setting
> didn't succeed. Or application authors will ignore the return value
> assuming that it always succeeded. Or read the value back to see if it
> succeeded. All icky, when we can well have a good return value.
> 
> > > There are two possible ways to handle this:
> > > a) accept anything that begins with a valid number.
> > > b) reject anything that isn't *only* a number
> > 
> > c) accept anything that is number, ignore newlines.

echo 255> brightness works, returns success.
echo 255 > brightness works but then returns -EINVAL.

So we currently do b, quite strictly. Its the trailing space thats the
problem. It also shouldn't have altered the brightness value if it ends
up returning -EINVAL.

I've looked around other implementations and it would appear we should
accept an optional space. Most sysfs attributes seem to handle this
differently, each with its own "bugs".

I've some fixes in mind both for the led and backlight classes which
I'll post once I've done a little more testing. I'd be interested to
know the official view on what the attributes should/shouldn't accept
is.

Cheers,

Richard

Re: led_class: storing a value can act but return -EINVAL

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]

On Sun, 2006-04-30 at 12:57 +0100, Richard Purdie wrote:

> echo 255> brightness works, returns success.

??
For me (bash) that doesn't do anything useful.
Were you looking for "echo -n 255 > brightness"?

> echo 255 > brightness works but then returns -EINVAL.

> So we currently do b, quite strictly. Its the trailing space thats the
> problem. It also shouldn't have altered the brightness value if it ends
> up returning -EINVAL.

Yes, but you do change the actual value, which IMHO you shouldn't when
it will return -EINVAL. I should have said

b) reject anything that isn't *only* a number and take no action

instead.

> I've looked around other implementations and it would appear we should
> accept an optional space. Most sysfs attributes seem to handle this
> differently, each with its own "bugs".

Yeah, unfortunately that is true. Maybe there should've been helper
functions like when you have a sysfs-int attribute that is set directly
without get/set calls. I'd suggest looking at that code.

> I've some fixes in mind both for the led and backlight classes which
> I'll post once I've done a little more testing. I'd be interested to
> know the official view on what the attributes should/shouldn't accept
> is.

I have a question about the backlight class: I'm writing a patch
currently to control the *keyboard* backlight on powerbooks, is that
appropriate for the backlight class (setting the fbdev callback to
reject everything)?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

Re: led_class: storing a value can act but return -EINVAL

[Pavel Machek]

Hi!

> > Well, I'd argue current behaviour is okay... can you strace it? It
> > should accept the number (return 3) then return -EINVAL.
> 
> That's exactly what happens.

And that's pretty much exactly okay.

You got success return when you wrote "255" there, then you got
-EINVAL when you tried to put newline there. Expected behaviour, I'd
say.

...I can see it looks ugly when you do echo manually, but you simply
should not do that.

> Which is totally bogus, because userspace will think that the setting
> didn't succeed. Or application authors will ignore the return value
> assuming that it always succeeded. Or read the value back to see if it
> succeeded. All icky, when we can well have a good return value.

Well, they got what they deserve, that \n does not belong there. And
they _did_ get success report -- "3" was returned. That's how unix
works, I'd say. Educate userspace authors that adding \n is bad idea.

							Pavel
-- 
Thanks for all the (sleeping) penguins.