Hi Greg,
Here's an updated version of my hotplug environment variable patch.
Based on Paul Jackson's suggestion, I switched the helper from a macro
to a function.
When I wanted to implement some environment variables in my hotplug
method, I looked for an example of how to do it. I noticed two
things: adding values ends up being kind of messy, and the hotplug
method in drivers/usb/core/usb.c is subtly wrong! These two patches
attempt to fix both of those problems.
If you apply these, I'll send patches to use add_hotplug_env_var() in
net/, drivers/ieee1394/, etc.
Thanks,
Roland
Add a (non-inlined) add_hotplug_env_var() function to <linux/kobject.h>
and lib/kobject.c. There's a lot of boilerplate code involved in
setting environment variables in a hotplug method, so we should have a
convenience function to consolidate it (and avoid subtle bugs).
Signed-off-by: Roland Dreier <[email protected]>
Index: linux-bk/include/linux/kobject.h
===================================================================
--- linux-bk.orig/include/linux/kobject.h 2004-09-28 09:51:09.000000000 -0700
+++ linux-bk/include/linux/kobject.h 2004-09-28 09:50:49.000000000 -0700
@@ -136,6 +136,32 @@
/**
+ * add_hotplug_env_var - helper for creating hotplug environment variables
+ * @envp: Pointer to table of environment variables, as passed into
+ * hotplug() method.
+ * @num_envp: Number of environment variable slots available, as
+ * passed into hotplug() method.
+ * @cur_index: Pointer to current index into @envp. It should be
+ * initialized to 0 before the first call to add_hotplug_env_var(),
+ * and will be incremented on success.
+ * @buffer: Pointer to buffer for environment variables, as passed
+ * into hotplug() method.
+ * @buffer_size: Length of @buffer, as passed into hotplug() method.
+ * @cur_len: Pointer to current length of space used in @buffer.
+ * Should be initialized to 0 before the first call to
+ * add_hotplug_env_var(), and will be incremented on success.
+ * @format: Format for creating environment variable (of the form
+ * "XXX=%x") for snprintf().
+ *
+ * Returns 0 if environment variable was added successfully or -ENOMEM
+ * if no space was available.
+ */
+extern int add_hotplug_env_var(char **envp, int num_envp, int *cur_index,
+ char *buffer, int buffer_size, int *cur_len,
+ const char *format, ...)
+ __attribute__((format (printf, 7, 8)));
+
+/**
* Use this when initializing an embedded kset with no other
* fields to initialize.
*/
Index: linux-bk/lib/kobject.c
===================================================================
--- linux-bk.orig/lib/kobject.c 2004-09-28 09:30:17.000000000 -0700
+++ linux-bk/lib/kobject.c 2004-09-28 10:45:49.000000000 -0700
@@ -232,11 +232,51 @@
if (top_kobj->kset && top_kobj->kset->hotplug_ops)
kset_hotplug(action, top_kobj->kset, kobj);
}
-#else
+
+int add_hotplug_env_var(char **envp, int num_envp, int *cur_index,
+ char *buffer, int buffer_size, int *cur_len,
+ const char *format, ...)
+{
+ va_list args;
+
+ /*
+ * We check against num_envp - 1 to make sure there is at
+ * least one slot left after we return, since the hotplug
+ * method needs to set the last slot to NULL.
+ */
+ if (*cur_index >= num_envp - 1)
+ 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;
+ va_end(args);
+
+ if (*cur_len > buffer_size)
+ return -ENOMEM;
+
+ (*cur_index)++;
+ return 0;
+}
+EXPORT_SYMBOL(add_hotplug_env_var);
+
+#else /* CONFIG_HOTPLUG */
+
void kobject_hotplug(const char *action, struct kobject *kobj)
{
return;
}
+
+int add_hotplug_env_var(char **envp, int num_envp, int *cur_index,
+ char *buffer, int buffer_size, int *cur_len,
+ const char *format, ...)
+{
+ return 0;
+}
+EXPORT_SYMBOL(add_hotplug_env_var);
#endif /* CONFIG_HOTPLUG */
/**
Use the new add_hotplug_env_var() function in drivers/usb/core/usb.c.
In addition to cleaning up the code, this fixes a (probably harmless)
bug here: for each value added to the environment, the code did
length += sprintf(...);
and then
scratch += length;
which means that we skip the sum of the lengths of all the values
we've put so far, rather than just the length of the value we just
put. This is probably harmless since we're unlikely to run out of
space but if nothing else it's setting a bad example....
I've tested this on a system with USB floppy and CD-ROM; hotplug gets
the same environment with the patch as without.
Signed-off-by: Roland Dreier <[email protected]>
Index: linux-bk/drivers/usb/core/usb.c
===================================================================
--- linux-bk.orig/drivers/usb/core/usb.c 2004-09-28 09:25:55.000000000 -0700
+++ linux-bk/drivers/usb/core/usb.c 2004-09-28 10:19:22.000000000 -0700
@@ -564,7 +564,6 @@
{
struct usb_interface *intf;
struct usb_device *usb_dev;
- char *scratch;
int i = 0;
int length = 0;
@@ -591,8 +590,6 @@
return -ENODEV;
}
- scratch = buffer;
-
#ifdef CONFIG_USB_DEVICEFS
/* If this is available, userspace programs can directly read
* all the device descriptors we don't tell them about. Or
@@ -600,37 +597,30 @@
*
* FIXME reduce hardwired intelligence here
*/
- envp [i++] = scratch;
- length += snprintf (scratch, buffer_size - length,
- "DEVICE=/proc/bus/usb/%03d/%03d",
- usb_dev->bus->busnum, usb_dev->devnum);
- if ((buffer_size - length <= 0) || (i >= num_envp))
+ if (add_hotplug_env_var(envp, num_envp, &i,
+ buffer, buffer_size, &length,
+ "DEVICE=/proc/bus/usb/%03d/%03d",
+ usb_dev->bus->busnum, usb_dev->devnum))
return -ENOMEM;
- ++length;
- scratch += length;
#endif
/* per-device configurations are common */
- envp [i++] = scratch;
- length += snprintf (scratch, buffer_size - length, "PRODUCT=%x/%x/%x",
- usb_dev->descriptor.idVendor,
- usb_dev->descriptor.idProduct,
- usb_dev->descriptor.bcdDevice);
- if ((buffer_size - length <= 0) || (i >= num_envp))
+ if (add_hotplug_env_var(envp, num_envp, &i,
+ buffer, buffer_size, &length,
+ "PRODUCT=%x/%x/%x",
+ usb_dev->descriptor.idVendor,
+ usb_dev->descriptor.idProduct,
+ usb_dev->descriptor.bcdDevice))
return -ENOMEM;
- ++length;
- scratch += length;
/* class-based driver binding models */
- envp [i++] = scratch;
- length += snprintf (scratch, buffer_size - length, "TYPE=%d/%d/%d",
- usb_dev->descriptor.bDeviceClass,
- usb_dev->descriptor.bDeviceSubClass,
- usb_dev->descriptor.bDeviceProtocol);
- if ((buffer_size - length <= 0) || (i >= num_envp))
+ if (add_hotplug_env_var(envp, num_envp, &i,
+ buffer, buffer_size, &length,
+ "TYPE=%d/%d/%d",
+ usb_dev->descriptor.bDeviceClass,
+ usb_dev->descriptor.bDeviceSubClass,
+ usb_dev->descriptor.bDeviceProtocol))
return -ENOMEM;
- ++length;
- scratch += length;
if (usb_dev->descriptor.bDeviceClass == 0) {
struct usb_host_interface *alt = intf->cur_altsetting;
@@ -639,18 +629,15 @@
* agents are called for all interfaces, and can use
* $DEVPATH/bInterfaceNumber if necessary.
*/
- envp [i++] = scratch;
- length += snprintf (scratch, buffer_size - length,
- "INTERFACE=%d/%d/%d",
- alt->desc.bInterfaceClass,
- alt->desc.bInterfaceSubClass,
- alt->desc.bInterfaceProtocol);
- if ((buffer_size - length <= 0) || (i >= num_envp))
+ if (add_hotplug_env_var(envp, num_envp, &i,
+ buffer, buffer_size, &length,
+ "INTERFACE=%d/%d/%d",
+ alt->desc.bInterfaceClass,
+ alt->desc.bInterfaceSubClass,
+ alt->desc.bInterfaceProtocol))
return -ENOMEM;
- ++length;
- scratch += length;
-
}
+
envp[i++] = NULL;
return 0;
Looks much improved to me - thanks.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.650.933.1373
On Tue, Sep 28, 2004 at 07:19:34PM -0700, Roland Dreier wrote:
> Add a (non-inlined) add_hotplug_env_var() function to <linux/kobject.h>
> and lib/kobject.c. There's a lot of boilerplate code involved in
> setting environment variables in a hotplug method, so we should have a
> convenience function to consolidate it (and avoid subtle bugs).
Cool. Well the code in kobject.c has changed a lot recently (see the
-mm tree) and the kernel-doc comments should be with the .c code, not
the header file, so here's the version I committed to my trees.
thanks,
greg k-h
===== include/linux/kobject.h 1.31 vs edited =====
--- 1.31/include/linux/kobject.h 2004-09-15 11:26:10 -07:00
+++ edited/include/linux/kobject.h 2004-09-29 16:51:02 -07:00
@@ -237,9 +237,17 @@
extern void subsys_remove_file(struct subsystem * , struct subsys_attribute *);
#ifdef CONFIG_HOTPLUG
-extern void kobject_hotplug(struct kobject *kobj, enum kobject_action action);
+void kobject_hotplug(struct kobject *kobj, enum kobject_action action);
+int add_hotplug_env_var(char **envp, int num_envp, int *cur_index,
+ char *buffer, int buffer_size, int *cur_len,
+ const char *format, ...)
+ __attribute__((format (printf, 7, 8)));
#else
static inline void kobject_hotplug(struct kobject *kobj, enum kobject_action action) { }
+static inline int add_hotplug_env_var(char **envp, int num_envp, int *cur_index,
+ char *buffer, int buffer_size, int *cur_len,
+ const char *format, ...)
+{ return 0; }
#endif
#endif /* __KERNEL__ */
===== lib/kobject_uevent.c 1.4 vs edited =====
--- 1.4/lib/kobject_uevent.c 2004-09-15 14:15:24 -07:00
+++ edited/lib/kobject_uevent.c 2004-09-29 16:54:19 -07:00
@@ -290,6 +290,56 @@
return;
}
EXPORT_SYMBOL(kobject_hotplug);
-#endif /* CONFIG_HOTPLUG */
+/**
+ * add_hotplug_env_var - helper for creating hotplug environment variables
+ * @envp: Pointer to table of environment variables, as passed into
+ * hotplug() method.
+ * @num_envp: Number of environment variable slots available, as
+ * passed into hotplug() method.
+ * @cur_index: Pointer to current index into @envp. It should be
+ * initialized to 0 before the first call to add_hotplug_env_var(),
+ * and will be incremented on success.
+ * @buffer: Pointer to buffer for environment variables, as passed
+ * into hotplug() method.
+ * @buffer_size: Length of @buffer, as passed into hotplug() method.
+ * @cur_len: Pointer to current length of space used in @buffer.
+ * Should be initialized to 0 before the first call to
+ * add_hotplug_env_var(), and will be incremented on success.
+ * @format: Format for creating environment variable (of the form
+ * "XXX=%x") for snprintf().
+ *
+ * Returns 0 if environment variable was added successfully or -ENOMEM
+ * if no space was available.
+ */
+int add_hotplug_env_var(char **envp, int num_envp, int *cur_index,
+ char *buffer, int buffer_size, int *cur_len,
+ const char *format, ...)
+{
+ va_list args;
+
+ /*
+ * We check against num_envp - 1 to make sure there is at
+ * least one slot left after we return, since the hotplug
+ * method needs to set the last slot to NULL.
+ */
+ if (*cur_index >= num_envp - 1)
+ 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;
+ va_end(args);
+ if (*cur_len > buffer_size)
+ return -ENOMEM;
+
+ (*cur_index)++;
+ return 0;
+}
+EXPORT_SYMBOL(add_hotplug_env_var);
+
+#endif /* CONFIG_HOTPLUG */
On Tue, Sep 28, 2004 at 07:19:34PM -0700, Roland Dreier wrote:
> Use the new add_hotplug_env_var() function in drivers/usb/core/usb.c.
> In addition to cleaning up the code, this fixes a (probably harmless)
> bug here: for each value added to the environment, the code did
>
> length += sprintf(...);
>
> and then
>
> scratch += length;
>
> which means that we skip the sum of the lengths of all the values
> we've put so far, rather than just the length of the value we just
> put. This is probably harmless since we're unlikely to run out of
> space but if nothing else it's setting a bad example....
>
> I've tested this on a system with USB floppy and CD-ROM; hotplug gets
> the same environment with the patch as without.
Applied, thanks.
greg k-h
Greg> Cool. Well the code in kobject.c has changed a lot recently
Greg> (see the -mm tree) and the kernel-doc comments should be
Greg> with the .c code, not the header file, so here's the version
Greg> I committed to my trees.
Cool, looks good to me.
- R.