Re: [PATCH 15/18] ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices

Ian Pilcher
2021-09-05T15:33:11Z

On 9/5/21 9:51 AM, Greg KH wrote:
>> It's really more of an error message for the system administrator. So
>> as with my earlier note, dev_info() would be my preference.
>
> Nope, dev_err() for real errors please.

Just to clarify, the error in this case is the system administrator
writing an incorrect value to a sysfs attribute (likely via a udev
rule), i.e. a "pilot error."

One of the reviewers of one of my RFC patch sets commented that those
should be INFO level at most.

So dev_err() or dev_info() for that sort of thing (always given that
only the root user has permission to write to trigger the error
message)?

>> The blkdev_skip_space() and blkdev_find_space() calls effectively find
>> the first non-whitespace token in the buffer (disk_name) and its length
>> (name_len). If the buffer only contains whitespace (e.g. echo > $ATTR),
>> then name_len will be 0.
>
> That's a crazy interface, as others pointed out, don't do that please.

As Pavel noted, it would be ideal to use symlink()/unlink() in the LED's
block_devices directory for this. As far as I know however, sysfs
doesn't support doing that. I'd be happy to learn otherwise. I would
also welcome any other suggestions for a better interface for setting up
the many-to-many relationships that the trigger supports.

That said, I don't know what that has to do with blkdev_skip_space() and
blkdev_find_space(), which are just helper functions that I use to parse
the device name out of the buffer passed to the store function.
Ultimately, the store function does need to handle the case where the
system administrator (or a broken udev rule) writes an all-whitespace
string to the attribute.

I will try to restructure the code to make things more clear.

Thanks!

--
========================================================================
In Soviet Russia, Google searches you!
========================================================================