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

[Greg KH <gregkh@linuxfoundation.org>]

On Sun, Sep 05, 2021 at 10:33:08AM -0500, Ian Pilcher wrote:
> 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)?

Really you should not have any kernel log messages for invalid data sent
to a sysfs file, just return -EINVAL and be done with it.

> > > 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.

sysfs does not allow that as that is not what sysfs is for.  Perhaps you
want to use configfs, as that is exactly what that is for.

> 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.

Handling invalid data is fine, but having to parse multiple values in a
single sysfs file violates the rules of sysfs.  Please use something
else instead.

thanks,

greg k-h