Skip to contentSkip to conversation

Hotplug variable patches

PATCH Superseded Roland Dreier 1 participant 3 messages Sep 13, 2004

Subsystems (2)
USB Driver core
lore
Share & tools
lore

Roland Dreier

author
#0 lore raw

cover Hotplug variable patches

Hi Greg,

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.

Let me know if you don't like the HOTPLUG_ENV_VAR name and want something different.

If you apply these, I'll send patches to use HOTPLUG_ENV_VAR in net/, drivers/ieee1394/, etc.

Thanks,
Roland

author
#1 lore raw

PATCH 1/2 kobject: add HOTPLUG_ENV_VAR

Add a HOTPLUG_ENV_VAR macro to <linux/kobject.h>. There's a lot of boilerplate code involved in setting environment variables in a hotplug method, so we should have a convenience macro to consolidate it (and avoid subtle bugs).

Signed-off-by Roland Dreier <[email protected]>
include/linux/kobject.h +14 -0
Index: linux-bk/include/linux/kobject.h
===================================================================
@@ -95,6 +95,20 @@
 			int num_envp, char *buffer, int buffer_size);
 };
 
+#define HOTPLUG_ENV_VAR(_buf, _size, _envp, _nenvp, _index, _fmt, ...)	\
+	({								\
+		int len, ret = 0;					\
+		_envp[_index++] = _buf;					\
+		len = snprintf(_buf, _size, _fmt, ## __VA_ARGS__);	\
+		if (_size - len <= 0 || _index >= _nenvp)		\
+			ret = -ENOMEM;					\
+		else {							\
+			_buf  += len + 1;				\
+			_size -= len + 1;				\
+		}							\
+		ret;							\
+	})
+
 struct kset {
 	struct subsystem	* subsys;
 	struct kobj_type	* ktype;
author
#2 lore raw

PATCH 2/2 USB: use HOTPLUG_ENV_VAR in core/usb.c

Use the new HOTPLUG_ENV_VAR macro is 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]>
drivers/usb/core/usb.c +14 -32
Index: linux-bk/drivers/usb/core/usb.c
===================================================================
@@ -564,9 +564,7 @@
 {
 	struct usb_interface *intf;
 	struct usb_device *usb_dev;
-	char *scratch;
 	int i = 0;
-	int length = 0;
 
 	if (!dev)
 		return -ENODEV;

@@ -591,8 +589,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 +596,27 @@
 	 *
 	 * FIXME reduce hardwired intelligence here
 	 */
-	envp [i++] = scratch;
-	length += snprintf (scratch, buffer_size - length,
+	if (HOTPLUG_ENV_VAR(buffer, buffer_size, envp, num_envp, i,
 			    "DEVICE=/proc/bus/usb/%03d/%03d",
-			    usb_dev->bus->busnum, usb_dev->devnum);
-	if ((buffer_size - length <= 0) || (i >= num_envp))
+			    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",
+	if (HOTPLUG_ENV_VAR(buffer, buffer_size, envp, num_envp, i,
+			    "PRODUCT=%x/%x/%x",
 			    usb_dev->descriptor.idVendor,
 			    usb_dev->descriptor.idProduct,
-			    usb_dev->descriptor.bcdDevice);
-	if ((buffer_size - length <= 0) || (i >= num_envp))
+			    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",
+	if (HOTPLUG_ENV_VAR(buffer, buffer_size, envp, num_envp, i,
+			    "TYPE=%d/%d/%d",
 			    usb_dev->descriptor.bDeviceClass,
 			    usb_dev->descriptor.bDeviceSubClass,
-			    usb_dev->descriptor.bDeviceProtocol);
-	if ((buffer_size - length <= 0) || (i >= num_envp))
+			    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 +625,14 @@
 		 * 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 (HOTPLUG_ENV_VAR(buffer, buffer_size, envp, num_envp, i,
+				    "INTERFACE=%d/%d/%d",
+				    alt->desc.bInterfaceClass,
+				    alt->desc.bInterfaceSubClass,
+				    alt->desc.bInterfaceProtocol))
 			return -ENOMEM;
-		++length;
-		scratch += length;
-
 	}
+
 	envp[i++] = NULL;
 
 	return 0;