linux-leds.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Ian Pilcher <arequipeno@gmail.com>
Cc: pavel@ucw.cz, linux-leds@vger.kernel.org,
	linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org,
	kabel@kernel.org
Subject: Re: [PATCH v4 2/2] leds: trigger: Add block device LED trigger
Date: Fri, 17 Sep 2021 06:53:26 +0100	[thread overview]
Message-ID: <YUQtVmwV90uab4/V@infradead.org> (raw)
In-Reply-To: <20210916202127.1216994-3-arequipeno@gmail.com>

On Thu, Sep 16, 2021 at 03:21:27PM -0500, Ian Pilcher wrote:
> +/* Block device information (BDI) - 1 per blkdev linked to at least 1 LED */
> +struct led_bdev_bdi {

It might be a good idea to pick a differene name.  BDI is a commonly
used shortcut for the backing device information used all over the block
layer and writeback code.

> +/* For many-to-many relationships between block devices and LEDs */
> +struct led_bdev_link {
> +	struct hlist_node	bdi_leds_node;
> +	struct hlist_node	led_bdis_node;
> +	struct led_bdev_bdi	*bdi;
> +	struct led_bdev_led	*led;
> +};


Why not just use a xarray to link them which due to the non-invasive
nature gets you n:m links "for free".

> +/* Forward declarations to make this file compile in a reasonable order */
> +static void led_bdev_process(struct work_struct *work);
> +static struct led_bdev_bdi *led_bdev_get_bdi(const char *buf, size_t size);
> +static struct block_device *led_bdev_get(const char *buf, size_t size,
> +					 fmode_t mode);
> +static int led_bdev_link(struct led_bdev_led *led, struct led_bdev_bdi *bdi);
> +static void led_bdev_put_bdi(struct led_bdev_bdi *bdi);
> +static void led_bdev_bdi_release(struct device *dev, void *res);
> +static void led_bdev_unlink(struct led_bdev_led *led,
> +			    struct led_bdev_link *link,
> +			    struct led_bdev_bdi *bdi, bool releasing);
> +static void led_bdev_update_bdi(struct led_bdev_bdi *bdi);
> +static bool led_bdev_blink(const struct led_bdev_led *led,
> +			   const struct led_bdev_bdi *bdi);

I seriously question the "reasonable" above if you need that many
forward declarations in brand new code.

> +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.

      reply	other threads:[~2021-09-17  5:55 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-16 20:21 [PATCH v4 0/2] Introduce block device LED trigger Ian Pilcher
2021-09-16 20:21 ` [PATCH v4 1/2] docs: Add block device (blkdev) LED trigger documentation Ian Pilcher
2021-09-17  6:19   ` Greg KH
2021-09-17 20:46     ` Ian Pilcher
2021-09-18  7:07       ` Greg KH
2021-09-18 14:43         ` Ian Pilcher
2021-09-20  6:43         ` Christoph Hellwig
2021-10-05 12:24           ` Marek Behún
2021-09-16 20:21 ` [PATCH v4 2/2] leds: trigger: Add block device LED trigger Ian Pilcher
2021-09-17  5:53   ` Christoph Hellwig [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YUQtVmwV90uab4/V@infradead.org \
    --to=hch@infradead.org \
    --cc=arequipeno@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=kabel@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=pavel@ucw.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).