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

Ian Pilcher
2021-09-04T22:35:32Z

On 9/4/21 12:59 AM, Greg KH wrote:
> DEVICE_ATTR_RO()? Or something like that? Do not use __ATTR() for
> device attributes if at all possible, worst case, use DEVICE_ATTR()
> here.

For some reason, it didn't click until now that these are device
attributes (because I was focused on the fact that I was working on the
LED trigger).

DEVICE_ATTR*() it is.

> And the mode settings are odd, are you sure you want that?

Yes. These are write-only attributes.

>> + if (name_len == 0) {
>> + pr_info("blkdev LED: empty block device name\n");
>
> Looks like debugging code, please remove.

It's really more of an error message for the system administrator. So
as with my earlier note, dev_info() would be my preference.

> And how can this ever happen?

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.

>
>> + return -EINVAL;
>> + }
>> +
>> + if (attr == &ledtrig_blkdev_attr_del) {
>> + blkdev_disk_delete(led, disk_name, name_len);
>> + } else { /* attr == &ledtrig_blkdev_attr_add */
>> + ret = blkdev_disk_add(led, disk_name, name_len);
>
> Why do you have a single attribute callback that does two totally
> different things? Just have 2 different callback functions please, it
> makes things much easier to review and maintain over time.

Hmmm. All of the "real work" is done in blkdev_disk_delete() and
blkdev_disk_add(). The store function's only purpose is to parse the
token(s) from the buffer, and that logic is exactly the same for the
two different attributes.

So it's a choice between:

> static ssize_t blkdev_add_or_del(struct device *const dev,
> struct device_attribute *const attr,
> const char *const buf, const size_t count)
> {
> struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev);
> const char *const disk_name = blkdev_skip_space(buf);
> const char *const endp = blkdev_find_space(disk_name);
> const ptrdiff_t name_len = endp - disk_name; /* always >= 0 */
> int ret;
>
> if (name_len == 0) {
> pr_info("blkdev LED: empty block device name\n");
> return -EINVAL;
> }
>
> if (attr == &ledtrig_blkdev_attr_del) {
> blkdev_disk_delete(led, disk_name, name_len);
> } else { /* attr == &ledtrig_blkdev_attr_add */
> ret = blkdev_disk_add(led, disk_name, name_len);
> if (ret != 0)
> return ret;
> }
>
> /*
> * Consume everything up to the next non-whitespace token (or the end
> * of the input). Avoids "empty block device name" error if there is
> * whitespace (such as a newline) after the last token.
> */
> return blkdev_skip_space(endp) - buf;
> }

Or:

> static ssize_t blkdev_add(struct device *const dev,
> struct device_attribute *const attr,
> const char *const buf, const size_t count)
> {
> struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev);
> const char *const disk_name = blkdev_skip_space(buf);
> const char *const endp = blkdev_find_space(disk_name);
> const ptrdiff_t name_len = endp - disk_name; /* always >= 0 */
> int ret;
>
> if (name_len == 0) {
> pr_info("blkdev LED: empty block device name\n");
> return -EINVAL;
> }
>
> ret = blkdev_disk_add(led, disk_name, name_len);
> if (ret != 0)
> return ret;
>
> /*
> * Consume everything up to the next non-whitespace token (or the end
> * of the input). Avoids "empty block device name" error if there is
> * whitespace (such as a newline) after the last token.
> */
> return blkdev_skip_space(endp) - buf;
> }
>
>
> static ssize_t blkdev_del(struct device *const dev,
> struct device_attribute *const attr,
> const char *const buf, const size_t count)
> {
> struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev);
> const char *const disk_name = blkdev_skip_space(buf);
> const char *const endp = blkdev_find_space(disk_name);
> const ptrdiff_t name_len = endp - disk_name; /* always >= 0 */
> int ret;
>
> if (name_len == 0) {
> pr_info("blkdev LED: empty block device name\n");
> return -EINVAL;
> }
>
> blkdev_disk_delete(led, disk_name, name_len);
>
> /* See comment in blkdev_add() */
> return blkdev_skip_space(endp) - buf;
> }

Maybe the right thing to do is to simply add a comment to clarify the
separation (and maybe rename the function as well)?

Thanks!

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