Re: [PATCH v4 1/2] docs: Add block device (blkdev) LED trigger documentation

[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@xxxxxxxxx>
> > +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!
========================================================================