Date:   Fri, 17 Sep 2021 15:46:55 -0500
From:   Ian Pilcher <arequipeno@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>, hch@...radead.org
Cc:     pavel@....cz, linux-leds@...r.kernel.org,
        linux-kernel@...r.kernel.org, kabel@...nel.org
Subject: Re: [PATCH v4 1/2] docs: Add block device (blkdev) LED trigger
 documentation

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@...il.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!
========================================================================

Powered by blists - more mailing lists