From: Greg KH <firstname.lastname@example.org>
To: Ian Pilcher <email@example.com>
Cc: firstname.lastname@example.org, email@example.com, firstname.lastname@example.org,
Subject: Re: [PATCH 15/18] ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices
Date: Sun, 5 Sep 2021 18:43:37 +0200 [thread overview]
Message-ID: <YTTzuVHBDgT2Hv9r@kroah.com> (raw)
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
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
next prev parent reply other threads:[~2021-09-05 16:43 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-03 20:45 [PATCH 00/18] Introduce block device LED trigger Ian Pilcher
2021-09-03 20:45 ` [PATCH 01/18] docs: Add block device (blkdev) LED trigger documentation Ian Pilcher
2021-09-04 6:29 ` Pavel Machek
2021-09-05 14:49 ` Ian Pilcher
2021-09-05 18:42 ` Pavel Machek
2021-09-05 23:13 ` Ian Pilcher
2021-09-03 20:45 ` [PATCH 02/18] ledtrig-blkdev: Add build infra for block device LED trigger Ian Pilcher
2021-09-03 20:45 ` [PATCH 03/18] ledtrig-blkdev: Add function placeholders needed by block changes Ian Pilcher
2021-09-04 16:57 ` kernel test robot
2021-09-03 20:45 ` [PATCH 04/18] block: Add block device LED trigger integrations Ian Pilcher
2021-09-03 20:45 ` [PATCH 05/18] ledtrig-blkdev: Implement functions called from block subsystem Ian Pilcher
2021-09-03 20:45 ` [PATCH 06/18] ledtrig-blkdev: Add function to get gendisk by name Ian Pilcher
2021-09-03 20:45 ` [PATCH 07/18] ledtrig-blkdev: Add constants, data types, and global variables Ian Pilcher
2021-09-03 20:45 ` [PATCH 08/18] ledtrig-blkdev: Add miscellaneous helper functions Ian Pilcher
2021-09-04 6:00 ` Greg KH
2021-09-04 22:43 ` Ian Pilcher
2021-09-03 20:45 ` [PATCH 09/18] ledtrig-blkdev: Periodically check devices for activity & blink LEDs Ian Pilcher
2021-09-04 6:01 ` Greg KH
2021-09-05 14:39 ` Ian Pilcher
2021-09-05 14:51 ` Greg KH
2021-09-05 14:56 ` Ian Pilcher
2021-09-05 15:12 ` Greg KH
2021-09-05 16:55 ` Eric Biggers
2021-09-03 20:45 ` [PATCH 10/18] ledtrig-blkdev: Add function to associate the trigger with an LED Ian Pilcher
2021-09-03 20:45 ` [PATCH 11/18] ledtrig-blkdev: Add function to associate a device " Ian Pilcher
2021-09-03 20:45 ` [PATCH 12/18] ledtrig-blkdev: Add function to remove LED/device association Ian Pilcher
2021-09-03 20:45 ` [PATCH 13/18] ledtrig-blkdev: Add function to disassociate a device from all LEDs Ian Pilcher
2021-09-03 20:45 ` [PATCH 14/18] ledtrig-blkdev: Add function to disassociate an LED from the trigger Ian Pilcher
2021-09-03 20:45 ` [PATCH 15/18] ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices Ian Pilcher
2021-09-04 5:57 ` Greg KH
2021-09-04 21:28 ` Ian Pilcher
2021-09-04 5:59 ` Greg KH
2021-09-04 22:35 ` Ian Pilcher
2021-09-05 14:51 ` Greg KH
2021-09-05 15:33 ` Ian Pilcher
2021-09-05 16:43 ` Greg KH [this message]
2021-09-03 20:45 ` [PATCH 16/18] ledtrig-blkdev: Add blink_time & interval sysfs attributes Ian Pilcher
2021-09-03 20:45 ` [PATCH 17/18] ledtrig-blkdev: Add mode (read/write/rw) sysfs attributue Ian Pilcher
2021-09-04 5:57 ` Greg KH
2021-09-04 21:01 ` Ian Pilcher
2021-09-05 14:50 ` Greg KH
2021-09-03 20:45 ` [PATCH 18/18] ledtrig-blkdev: Add initialization & exit functions Ian Pilcher
2021-09-04 6:35 ` [PATCH 00/18] Introduce block device LED trigger Pavel Machek
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).