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

[Ian Pilcher <arequipeno@gmail.com>]

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!
========================================================================