Messages in this thread | | | Date | Fri, 17 Sep 2021 15:46:55 -0500 | Subject | Re: [PATCH v4 1/2] docs: Add block device (blkdev) LED trigger documentation | From | Ian Pilcher <> |
| |
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! ========================================================================
| |