Re: [PATCH 01/99] lib: Add option iterator

From: Randy Dunlap <rdunlap@infradead.org>
To: Thomas Zimmermann <tzimmermann@suse.de>,
	deller@gmx.de, paulus@samba.org, benh@kernel.crashing.org,
	linux@armlinux.org.uk, pjones@redhat.com, timur@kernel.org,
	adaplas@gmail.com, s.hauer@pengutronix.de, shawnguo@kernel.org,
	mbroemme@libmpq.org, thomas@winischhofer.net,
	James.Bottomley@HansenPartnership.com, spock@gentoo.org,
	sudipm.mukherjee@gmail.com, teddy.wang@siliconmotion.com,
	geert+renesas@glider.be, corbet@lwn.net
Cc: linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 01/99] lib: Add option iterator
Date: Mon, 6 Mar 2023 14:37:39 -0800	[thread overview]
Message-ID: <570edde0-cfea-f560-fe83-6077f4f221e5@infradead.org> (raw)
In-Reply-To: <20230306160016.4459-2-tzimmermann@suse.de>

Hi,

On 3/6/23 07:58, Thomas Zimmermann wrote:
> Add struct option_iter and helpers that walk over individual options
> of an option string. Add documentation.
> 
> Kernel parameters often have the format of
> 
>   param=opt1,opt2:val,opt3
> 
> where the option string contains a number of comma-separated options.
> Drivers usually use strsep() in a loop to extract individual options
> from the string. Each call to strsep() modifies the given string, so
> callers have to duplicate kernel parameters that are to be parsed
> multiple times.
> 
> The new struct option_iter and its helpers wrap this code behind a
> clean interface. Drivers can iterate over the options without having
> to know the details of the option-string format. The iterator handles
> string memory internally without modifying the original options.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  Documentation/core-api/kernel-api.rst |  9 +++
>  include/linux/cmdline.h               | 29 ++++++++
>  lib/Makefile                          |  2 +-
>  lib/cmdline_iter.c                    | 97 +++++++++++++++++++++++++++
>  4 files changed, 136 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/cmdline.h
>  create mode 100644 lib/cmdline_iter.c
> 
> diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
> index 62f961610773..cdc7ba8decf9 100644
> --- a/Documentation/core-api/kernel-api.rst
> +++ b/Documentation/core-api/kernel-api.rst
> @@ -93,9 +93,18 @@ Bitmap Operations
>  Command-line Parsing
>  --------------------
>  
> +.. kernel-doc:: lib/cmdline_iter.c
> +   :doc: overview
> +
>  .. kernel-doc:: lib/cmdline.c
>     :export:
>  
> +.. kernel-doc:: lib/cmdline_iter.c
> +   :export:
> +
> +.. kernel-doc:: include/linux/cmdline.h
> +   :internal:
> +
>  Sorting
>  -------
>  
> diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
> new file mode 100644
> index 000000000000..5d7e648e98a5
> --- /dev/null
> +++ b/include/linux/cmdline.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef LINUX_CMDLINE_H
> +#define LINUX_CMDLINE_H
> +
> +/**
> + * struct option_iter - Iterates over string of kernel or module options
> + */
> +struct option_iter {
> +	char *optbuf;
> +	char *next_opt;
> +};
> +
> +void option_iter_init(struct option_iter *iter, const char *options);
> +void option_iter_release(struct option_iter *iter);
> +const char *option_iter_incr(struct option_iter *iter);
> +
> +/**
> + * option_iter_next - Loop condition to move over options
> + * @iter_:	the iterator
> + * @opt_:	the name of the option variable
> + *
> + * Iterates over option strings as part of a while loop and
> + * stores the current option in opt_.
> + */
> +#define option_iter_next(iter_, opt_) \
> +	(((opt_) = option_iter_incr(iter_)) != NULL)
> +
> +#endif

> diff --git a/lib/cmdline_iter.c b/lib/cmdline_iter.c
> new file mode 100644
> index 000000000000..d9371dfea08b
> --- /dev/null
> +++ b/lib/cmdline_iter.c
> @@ -0,0 +1,97 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/cmdline.h>
> +#include <linux/export.h>
> +#include <linux/slab.h>
> +
> +/**
> + * DOC: overview
> + *
> + * A kernel parameter's option string can contain multiple comma-separated
> + * options. Modules can parse an option string with struct &option_iter and
> + * its helpers. After obtaining the string, initialize and instance of the

                                                          an instance

> + * option iterator and loop iver its content as show below.

                               over

> + *
> + * .. code-block:: c
> + *
> + *	const char *options = ...; // provided option string
> + *
> + *	struct option_iter iter;
> + *	const char *opt;
> + *
> + *	option_iter_init(&iter, options);
> + *
> + *	while (option_iter_next(&iter, &opt)) {
> + *		if (!strcmp(opt, "foo"))
> + *			...
> + *		else (strcmp(opt, "bar"))
> + *			...
> + *		else
> + *			pr_warn("unknown option %s\n", opt);
> + *	}
> + *
> + *	option_iter_release(&iter);
> + *
> + * The call to option_iter_init() initializes the iterator instance
> + * from the option string. The while loop walks over the individual
> + * options in the sting and returns each in the second argument. The
> + * returned memory is owned by the iterator instance and callers may
> + * not modify or free it. The call to option_iter_release() frees all
> + * resources of the iterator. This process does not modify the original
> + * option string. If the option string contains an empty option (i.e.,
> + * two commas next to each other), option_iter_next() skips the empty
> + * option automatically.

Is that latter skipping over a ",," automatically something that you have
observed as needed?
I can imagine a driver or module wanting to know that an empty string
was entered (i.e., ",,").

> + */
> +
> +/**
> + * option_iter_init - Initializes an option iterator
> + * @iter:	the iterator to initialize
> + * @options:	the options string
> + */
> +void option_iter_init(struct option_iter *iter, const char *options)
> +{
> +	if (options && *options)
> +		iter->optbuf = kstrdup(options, GFP_KERNEL); // can be NULL
> +	else
> +		iter->optbuf = NULL;
> +	iter->next_opt = iter->optbuf;
> +}
> +EXPORT_SYMBOL(option_iter_init);
> +
> +/**
> + * option_iter_release - Releases an option iterator's resources
> + * @iter:	the iterator
> + */
> +void option_iter_release(struct option_iter *iter)
> +{
> +	kfree(iter->optbuf);
> +	iter->next_opt = NULL;
> +}
> +EXPORT_SYMBOL(option_iter_release);
> +
> +/**
> + * option_iter_incr - Return current option and advance to the next
> + * @iter:	the iterator
> + *
> + * Returns:

 * Return:
matches kernel-doc notation documentation.

> + * The current option string, or NULL if there are no more options.
> + */
> +const char *option_iter_incr(struct option_iter *iter)
> +{
> +	char *opt;
> +
> +	if (!iter->next_opt) { // can be OK if kstrdup failed
> +		if (iter->optbuf) // iter has already been released; logic error
> +			pr_err("Incrementing option iterator without string\n");
> +		return NULL;
> +	}
> +
> +	do {
> +		opt = strsep(&iter->next_opt, ",");
> +		if (!opt)
> +			return NULL;
> +	} while (!*opt); // found empty option string, try next
> +
> +	return opt;
> +}
> +EXPORT_SYMBOL(option_iter_incr);

Looks useful. Thanks.

-- 
~Randy