Hi Hans,
On Sun, 29 Jul 2007 09:13:03 +0200, Hans de Goede wrote:
> Howto check the validity of user written values to sysfs attributes:
>
> hwmon sysfs attributes always contain numbers, so the first thing to do is to
> convert the input to a number, there are 2 ways todo this depending whether
> the number can be negative or not:
> unsigned long u = simple_strtoul(buf, NULL, 10);
> long s = simple_strtol(buf, NULL, 10);
>
> With buf being the buffer with the user input being passed by the kernel.
> Notice that we do not use the second argument of strto[u]l, and thus cannot
> tell when 0 is returned, if this was really 0 or is caused by invalid input.
> This is done deliberately as checking this everywhere would add a lot of
> code to the kernel. We do need to document clearly that writing a non-number
> will be seen as writing 0.
This last sentence obviously can't be left verbatim in the final text.
> Notice that it is important to always store the converted value in an unsigned
> long or long, so that no wrap around can happen before any further checking.
>
> After conversion and storing the converted value in the right type, the value
"After converting and storing"?
> should be checked if its acceptable. Be carefull with further conversions on the
Spelling: careful.
> value before checking it for validity, as these conversions could still cause a
> wrap around before the check. For example do not multiply the result, and only add
> / substract if it has been divided before the add / substract.
Spelling: subtract (I had to double-check this one, I would have had it
wrong myself.)
> What to do if a value is found to be invalid, depends on the type of the sysfs
> attribute that is being set. If its a continuous setting like a tempX_max or
Spelling: it's.
> inX_max attribute, then the value should be clamped to its limits using
> SENSORS_LIMIT(value, min_limit, max_limit). If its not continuous like for
Ditto.
> example a tempX_type, then when an invalid value is written, -EINVAL should be
> returned.
>
> Example1, temp1_max, register presents -128 - 127 degrees as 0 - 255:
> long v = simple_strtol(buf, NULL, 10) / 1000;
> SENSORS_LIMIT(v, -128, 127);
> v += 128;
> /* write v to register */
This example is confusing IMHO, as most chips present -128 - 127
degrees as 0 - 255, but using a signed (2's complement) value, not
using an offset.
> Example2, fan divider setting, valid values 2, 4 and 8
> unsigned long v = simple_strtoul(buf, NULL, 10);
>
> switch (v) {
> case 2: v = 1; break;
> case 4: v = 2; break;
> case 8: v = 3; break;
> default:
> return -EINVAL;
> }
> /* write v to register */
>
> ---
>
> So, let me know what you think of the new version above and if its acceptable
> I'll do a patch adding this to the sysfs interface doc, or should it be in
> another doc?
sysfs-interface seems to be the right place.
--
Jean Delvare
