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

[Greg KH]

On Fri, Sep 17, 2021 at 03:46:55PM -0500, Ian Pilcher wrote:
> 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().

Just use the kernel block device name and that way you do not have to
parse anything as it is unique and no paths are having to be followed.

That's the way that other LED apis are working, right?

thanks,

greg k-h