Re: [patch] encapsulate uevent()/add_uevent_var() buffer handling

From: Cornelia Huck
Date: Mon Aug 13 2007 - 13:32:54 EST

On Mon, 13 Aug 2007 16:37:11 +0200,
Kay Sievers <kay.sievers@xxxxxxxx> wrote:

> Hi,
> this changes the uevent buffer stuff to use a struct instead
> of tons of parameters. It does no longer require the caller to
> do the proper buffer termination and size accounting, which is
> currently wrong in a lot of places.

I was working on a patch which tried to fix the same things but added
more parameters to uevent. Using a struct is really better.

>
> This tells everything:
> 47 files changed, 265 insertions(+), 605 deletions(-)

Cool.

>
> Thanks,
> Kay
>
> From: Kay Sievers <kay.sievers@xxxxxxxx>
> Subject: Driver core: encapsulate uevent()/add_uevent_var() buffer handling
>
> Signed-off-by: Kay Sievers <kay.sievers@xxxxxxxx>
> ---
> arch/ia64/sn/kernel/tiocx.c | 3
> arch/powerpc/kernel/of_device.c | 37 ++------
> arch/powerpc/kernel/vio.c | 14 ---
> arch/powerpc/platforms/ps3/system-bus.c | 9 -
> block/genhd.c | 35 +------
> drivers/acpi/scan.c | 16 +--
> drivers/amba/bus.c | 8 -
> drivers/base/class.c | 42 ++-------
> drivers/base/core.c | 83 +++++-------------
> drivers/base/firmware_class.c | 10 --
> drivers/base/memory.c | 3
> drivers/base/platform.c | 6 -
> drivers/eisa/eisa-bus.c | 9 -
> drivers/firewire/fw-device.c | 9 -
> drivers/firmware/dmi-id.c | 16 +--
> drivers/i2c/i2c-core.c | 8 -
> drivers/ide/ide.c | 17 +--
> drivers/ieee1394/nodemgr.c | 17 ---
> drivers/infiniband/core/sysfs.c | 9 -
> drivers/input/input.c | 62 ++++---------
> drivers/input/serio/serio.c | 11 --
> drivers/media/video/pvrusb2/pvrusb2-sysfs.c | 4
> drivers/misc/tifm_core.c | 9 -
> drivers/mmc/core/bus.c | 11 --
> drivers/pci/hotplug.c | 28 +-----
> drivers/pci/pci-driver.c | 3
> drivers/pci/pci.h | 3
> drivers/pcmcia/cs.c | 10 --
> drivers/pcmcia/ds.c | 26 +----
> drivers/power/power_supply.h | 3
> drivers/power/power_supply_sysfs.c | 16 +--
> drivers/s390/cio/ccwgroup.c | 3
> drivers/s390/cio/device.c | 18 +--
> drivers/s390/crypto/ap_bus.c | 14 ---
> drivers/scsi/scsi_sysfs.c | 9 -
> drivers/spi/spi.c | 7 -
> drivers/usb/core/driver.c | 29 +-----
> drivers/usb/core/message.c | 17 ---
> drivers/w1/w1.c | 18 +--
> include/asm-powerpc/of_device.h | 2
> include/linux/device.h | 15 +--
> include/linux/kobject.h | 23 +++--
> lib/kobject_uevent.c | 127 +++++++++-------------------
> net/atm/atm_sysfs.c | 7 -
> net/core/net-sysfs.c | 14 ---
> net/wireless/sysfs.c | 3
> sound/aoa/soundbus/core.c | 27 +----
> 47 files changed, 265 insertions(+), 605 deletions(-)
>

This is against current git? -mm has at least
drivers/mmc/core/sdio_bus.c in addition to convert, but that is
straightforward.

> diff --git a/arch/ia64/sn/kernel/tiocx.c b/arch/ia64/sn/kernel/tiocx.c
> index 5a289e4..a88eba3 100644
> --- a/arch/ia64/sn/kernel/tiocx.c
> +++ b/arch/ia64/sn/kernel/tiocx.c
> @@ -66,8 +66,7 @@ static int tiocx_match(struct device *dev, struct device_driver *drv)
>
> }
>
> -static int tiocx_uevent(struct device *dev, char **envp, int num_envp,
> - char *buffer, int buffer_size)
> +static int tiocx_uevent(struct device *dev, struct kobj_uevent_env *env)
> {
> return -ENODEV;
> }

I'm not sure whether this bus wants to use uevent_suppress instead, but
that would be a different patch.

> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 268e301..455e280 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -44,14 +44,12 @@ static int amba_match(struct device *dev, struct device_driver *drv)
> }
>
> #ifdef CONFIG_HOTPLUG
> -static int amba_uevent(struct device *dev, char **envp, int nr_env, char *buf, int bufsz)
> +static int amba_uevent(struct device *dev, struct kobj_uevent_env *env)
> {
> struct amba_device *pcdev = to_amba_device(dev);
> - int retval = 0, i = 0, len = 0;
> + int retval = 0;
>
> - retval = add_uevent_var(envp, nr_env, &i,
> - buf, bufsz, &len,
> - "AMBA_ID=%08x", pcdev->periphid);
> + retval = add_uevent_var(env, "AMBA_ID=%08x", pcdev->periphid);
> envp[i] = NULL;

Forgotten NULL-setting.

> return retval;
> }


> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index df02814..06ffeda 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -22,8 +22,6 @@
> #include <linux/kobject.h>
> #include <net/sock.h>
>
> -#define BUFFER_SIZE 2048 /* buffer for the variables */
> -#define NUM_ENVP 32 /* number of env pointers */
>
> /* the strings here must match the enum in include/linux/kobject.h */
> const char *kobject_actions[] = {
> @@ -54,31 +52,21 @@ static struct sock *uevent_sock;
> * corresponding error when it fails.
> */
> int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> - char *envp_ext[])
> + char *envp_ext[])
> {
> - char **envp;
> - char *buffer;
> - char *scratch;
> - const char *action_string;
> + struct kobj_uevent_env *env;
> + const char *action_string = kobject_actions[action];
> const char *devpath = NULL;
> const char *subsystem;
> struct kobject *top_kobj;
> struct kset *kset;
> struct kset_uevent_ops *uevent_ops;
> u64 seq;
> - char *seq_buff;
> int i = 0;
> int retval = 0;
> - int j;
>
> pr_debug("%s\n", __FUNCTION__);
>
> - action_string = kobject_actions[action];
> - if (!action_string) {
> - pr_debug("kobject attempted to send uevent without action_string!\n");
> - return -EINVAL;
> - }
> -

Hm, unrelated change?

> /* search the kset we belong to */
> top_kobj = kobj;
> while (!top_kobj->kset && top_kobj->parent) {
> @@ -92,7 +80,7 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> kset = top_kobj->kset;
> uevent_ops = kset->uevent_ops;
>
> - /* skip the event, if the filter returns zero. */
> + /* skip the event, if the filter returns zero. */
> if (uevent_ops && uevent_ops->filter)
> if (!uevent_ops->filter(kset, kobj)) {
> pr_debug("kobject filter function caused the event to drop!\n");
> @@ -109,18 +97,11 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> return 0;
> }
>
> - /* environment index */
> - envp = kzalloc(NUM_ENVP * sizeof (char *), GFP_KERNEL);
> - if (!envp)
> + /* environment buffer */
> + env = kzalloc(sizeof(struct kobj_uevent_env), GFP_KERNEL);
> + if (!env)
> return -ENOMEM;
>
> - /* environment values */
> - buffer = kmalloc(BUFFER_SIZE, GFP_KERNEL);
> - if (!buffer) {
> - retval = -ENOMEM;
> - goto exit;
> - }
> -
> /* complete object path */
> devpath = kobject_get_path(kobj, GFP_KERNEL);
> if (!devpath) {
> @@ -128,29 +109,19 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> goto exit;
> }
>
> - /* event environemnt for helper process only */
> - envp[i++] = "HOME=/";
> - envp[i++] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
> -
> /* default keys */
> - scratch = buffer;
> - envp [i++] = scratch;
> - scratch += sprintf(scratch, "ACTION=%s", action_string) + 1;
> - envp [i++] = scratch;
> - scratch += sprintf (scratch, "DEVPATH=%s", devpath) + 1;
> - envp [i++] = scratch;
> - scratch += sprintf(scratch, "SUBSYSTEM=%s", subsystem) + 1;
> - for (j = 0; envp_ext && envp_ext[j]; j++)
> - envp[i++] = envp_ext[j];
> - /* just reserve the space, overwrite it after kset call has returned */
> - envp[i++] = seq_buff = scratch;
> - scratch += strlen("SEQNUM=18446744073709551616") + 1;
> + add_uevent_var(env, "ACTION=%s", action_string);
> + add_uevent_var(env, "DEVPATH=%s", devpath);
> + add_uevent_var(env, "SUBSYSTEM=%s", subsystem);
> +
> + /* keys passed in from the caller */
> + if (envp_ext)
> + for (i = 0; envp_ext[i]; i++)
> + add_uevent_var(env, envp_ext[i]);
>
> /* let the kset specific function add its stuff */
> if (uevent_ops && uevent_ops->uevent) {
> - retval = uevent_ops->uevent(kset, kobj,
> - &envp[i], NUM_ENVP - i, scratch,
> - BUFFER_SIZE - (scratch - buffer));
> + retval = uevent_ops->uevent(kset, kobj, env);
> if (retval) {
> pr_debug ("%s - uevent() returned %d\n",
> __FUNCTION__, retval);
> @@ -158,11 +129,11 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> }
> }
>
> - /* we will send an event, request a new sequence number */
> + /* we will send an event, so request a new sequence number */
> spin_lock(&sequence_lock);
> seq = ++uevent_seqnum;
> spin_unlock(&sequence_lock);
> - sprintf(seq_buff, "SEQNUM=%llu", (unsigned long long)seq);
> + add_uevent_var(env, "SEQNUM=%llu", (unsigned long long)seq);
>
> #if defined(CONFIG_NET)
> /* send netlink message */
> @@ -172,17 +143,19 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
>
> /* allocate message with the maximum possible size */
> len = strlen(action_string) + strlen(devpath) + 2;
> - skb = alloc_skb(len + BUFFER_SIZE, GFP_KERNEL);
> + skb = alloc_skb(len + env->buflen, GFP_KERNEL);
> if (skb) {
> + char *scratch;
> +
> /* add header */
> scratch = skb_put(skb, len);
> sprintf(scratch, "%s@%s", action_string, devpath);
>
> /* copy keys to our continuous event payload buffer */
> - for (i = 2; envp[i]; i++) {
> - len = strlen(envp[i]) + 1;
> + for (i = 0; i < env->envp_idx; i++) {
> + len = strlen(env->envp[i]) + 1;
> scratch = skb_put(skb, len);
> - strcpy(scratch, envp[i]);
> + strcpy(scratch, env->envp[i]);
> }
>
> NETLINK_CB(skb).dst_group = 1;
> @@ -198,13 +171,15 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> argv [0] = uevent_helper;
> argv [1] = (char *)subsystem;
> argv [2] = NULL;
> - call_usermodehelper (argv[0], argv, envp, UMH_WAIT_EXEC);
> + add_uevent_var(env, "HOME=/");
> + add_uevent_var(env, "PATH=/sbin:/bin:/usr/sbin:/usr/bin");
> +
> + call_usermodehelper (argv[0], argv, env->envp, UMH_WAIT_EXEC);
> }
>
> exit:
> kfree(devpath);
> - kfree(buffer);
> - kfree(envp);
> + kfree(env);
> return retval;
> }
>
> @@ -227,52 +202,32 @@ int kobject_uevent(struct kobject *kobj, enum kobject_action action)
> EXPORT_SYMBOL_GPL(kobject_uevent);
>
> /**
> - * add_uevent_var - helper for creating event variables
> - * @envp: Pointer to table of environment variables, as passed into
> - * uevent() method.
> - * @num_envp: Number of environment variable slots available, as
> - * passed into uevent() method.
> - * @cur_index: Pointer to current index into @envp. It should be
> - * initialized to 0 before the first call to add_uevent_var(),
> - * and will be incremented on success.
> - * @buffer: Pointer to buffer for environment variables, as passed
> - * into uevent() method.
> - * @buffer_size: Length of @buffer, as passed into uevent() method.
> - * @cur_len: Pointer to current length of space used in @buffer.
> - * Should be initialized to 0 before the first call to
> - * add_uevent_var(), and will be incremented on success.
> - * @format: Format for creating environment variable (of the form
> - * "XXX=%x") for snprintf().
> + * add_uevent_var - add key value string to the environment buffer
> + * @env: environment buffer structure
> + * @format: printf format for the key=value pair
> *
> * Returns 0 if environment variable was added successfully or -ENOMEM
> * if no space was available.
> */
> -int add_uevent_var(char **envp, int num_envp, int *cur_index,
> - char *buffer, int buffer_size, int *cur_len,
> - const char *format, ...)
> +int add_uevent_var(struct kobj_uevent_env *env, const char *format, ...)
> {
> va_list args;
> + int len;
>
> - /*
> - * We check against num_envp - 1 to make sure there is at
> - * least one slot left after we return, since kobject_uevent()
> - * needs to set the last slot to NULL.
> - */
> - if (*cur_index >= num_envp - 1)
> + if (env->envp_idx >= ARRAY_SIZE(env->envp))
> return -ENOMEM;
>
> - envp[*cur_index] = buffer + *cur_len;
> -
> va_start(args, format);
> - *cur_len += vsnprintf(envp[*cur_index],
> - max(buffer_size - *cur_len, 0),
> - format, args) + 1;
> + len = vsnprintf(&env->buf[env->buflen],
> + sizeof(env->buf) - env->buflen,
> + format, args);
> va_end(args);
>
> - if (*cur_len > buffer_size)
> + if (len + 1 >= (sizeof(env->buf) - env->buflen))
> return -ENOMEM;
>
> - (*cur_index)++;
> + env->envp[env->envp_idx++] = &env->buf[env->buflen];
> + env->buflen += len + 1;
> return 0;
> }
> EXPORT_SYMBOL_GPL(add_uevent_var);

Looks good!
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/