[PATCH][0/2] [take 2] Hotplug variable patches

[PATCH][0/2] [take 2] Hotplug variable patches

[Roland Dreier]

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

[PATCH][1/2] [take 2] kobject: add add_hotplug_env_var

[Roland Dreier]

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 <roland@topspin.com>

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 */
 
 /**

[PATCH][2/2] [take 2] USB: use add_hotplug_env_var in core/usb.c

[Roland Dreier]

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 <roland@topspin.com>

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;

Re: [PATCH][0/2] [take 2] Hotplug variable patches

[Paul Jackson]

Looks much improved to me - thanks.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH][1/2] [take 2] kobject: add add_hotplug_env_var

[Greg KH]

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 */

Re: [PATCH][2/2] [take 2] USB: use add_hotplug_env_var in core/usb.c

[Greg KH]

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

Re: [PATCH][1/2] [take 2] kobject: add add_hotplug_env_var

[Roland Dreier]

    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.