lkml.org 
[lkml]   [2021]   [Sep]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v4 1/2] docs: Add block device (blkdev) LED trigger documentation
From
Combining 2 related threads ...

On 9/17/21 01:19, Greg KH wrote:
> On Thu, Sep 16, 2021 at 03:21:26PM -0500, Ian Pilcher wrote:
>> +What: /sys/class/leds/<led>/link_device
>> +Date: September 2021
>> +Contact: Ian Pilcher <arequipeno@gmail.com>
>> +Description:
>> + Associate a block device with this LED by writing the path to
>> + the device special file (e.g. /dev/sda) to this attribute.
>> + Symbolic links are followed. Optionally, the leading "/dev/"
>> + may be omitted.
>
> No, please don't follow symlinks, stick with kernel names here,
> otherwise you have a mismatch between that and the list of devices in
> this file:
>
>> +What: /sys/class/leds/<led>/linked_devices

I did update the documentation to mention that fact.

Following symlinks is the behavior of blkdev_get_by_path(), not some-
thing that my code is doing.

As far as using kernel names, that would be my preference, but I simply
don't know of any way to do so with the existing block API. To my
knowledge, there simply isn't anything like a blkdev_get_by_name() API.

This the reason that I added the "retry" logic to led_bdev_get(). It
doesn't prevent the system administrator from using a symbolic link (or
an oddly named special file), but it does make an unqualified name like
"sda" work if the expected special file exists in /dev.

However ...

On 9/17/21 00:53, Christoph Hellwig wrote:
> On Thu, Sep 16, 2021 at 03:21:27PM -0500, Ian Pilcher wrote:
>> +static struct block_device *led_bdev_get(const char *const buf,
>> + const size_t count, fmode_t mode)
>> +{
>> + static const char dev[] = "/dev/";
>> + struct block_device *bdev;
>> + char *dev_path, *path;
>> +
>> + /* sizeof(dev) includes terminating null */
>> + dev_path = kmalloc(sizeof(dev) + count, GFP_KERNEL);
>> + if (dev_path == NULL)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + /* sizeof(dev) - 1 is compile-time equivalent of strlen(dev) */
>> + memcpy(dev_path, dev, sizeof(dev) - 1);
>> + path = dev_path + sizeof(dev) - 1;
>> + memcpy(path, buf, count + 1); /* include terminating null */
>> + strim(path);
>> +
>> +try_blkdev_get:
>> + bdev = blkdev_get_by_path(path, mode, THIS_MODULE);
>> + if (IS_ERR(bdev) && PTR_ERR(bdev) == -ENOENT && path != dev_path) {
>> + path = dev_path;
>> + goto try_blkdev_get;
>> + }
>> +
>> + kfree(dev_path);
>> + return bdev;
>
> Please just required the user to put in the whole path and remove all
> this garbage. There is no need to build your own broken wrappers around
> the VFS path resolution.

Please be specific about what is broken.

If you see an actual bug in the code, please tell me what it is.

If (as I suspect) you disagree with the basic idea of retrying with
"/dev/" prepended to the supplied path, please say that.

Honestly, I wasn't particularly enthusiastic about it in the first
place; it feels like something that should be done in user space. I
wouldn't have included it if I didn't have to make a writable copy of
the buffer anyway, in order to trim a trailing newline.

I can certainly remove the re-check logic. The end result will be an
API that is slightly less "user friendly" in return for saving a bit of
pointer arithmetic and a 5-byte memcpy().

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

\
 
 \ /
  Last update: 2021-09-17 22:48    [W:0.151 / U:0.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site