(Resending because this seems to have gotten dropped)
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
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]>
Index: linux-bk/drivers/usb/core/usb.c
===================================================================
--- linux-bk.orig/drivers/usb/core/usb.c 2004-09-12 15:21:27.000000000 -0700
+++ linux-bk/drivers/usb/core/usb.c 2004-09-12 19:28:23.000000000 -0700
@@ -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;
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]>
Index: linux-bk/include/linux/kobject.h
===================================================================
--- linux-bk.orig/include/linux/kobject.h 2004-09-12 15:17:02.000000000 -0700
+++ linux-bk/include/linux/kobject.h 2004-09-12 19:44:31.000000000 -0700
@@ -95,6 +95,20 @@
int num_envp, char *buffer, int buffer_size);
};
+#define HOTPLUG_ENV_VAR(_buf, _size, _envp, _nenvp, _index, _fmt, _arg...) \
+ ({ \
+ int len, ret = 0; \
+ _envp[_index++] = _buf; \
+ len = snprintf(_buf, _size, _fmt, ## _arg); \
+ 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;
Roland wrote:
> Add a HOTPLUG_ENV_VAR macro ...
Would a procedure (not inlined) save text space here,
provide better type checking, and be easier to read?
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.650.933.1373
Paul> Would a procedure (not inlined) save text space here,
Paul> provide better type checking, and be easier to read?
The problem is that the straightforward way to implement this helper
modifies three of its parameters (the current buffer location, the
amount of space left, and the current index). It seemed ugly to force
these three parameters to be passed by address.
I could easily rework this to do as you suggest though. Greg, what's
your opinion?
Thanks,
Roland
Roland wrote:
> The problem is that the straightforward way to implement this helper
> modifies three of its parameters (the current buffer location, the
> amount of space left, and the current index).
You could reduce the number of modified parameters from three to two?
I find snprintf usage schemes that move the buffer location to be
confusing -- tend to lead to subtly wrong code ;). Better to keep
the invariants for each variable you use as simple as you can, and
the number of variables as few as you can. Keeping a variable constant
is one nice way to simplify its invariants ;).
For example, leave buffer unmoved, and only change one variable on each
snprintf - the count of how many chars would have been written, given
infinite space.
I've had good luck of late with the following way of calling snprintf
(example taken from kernel/cpuset.c):
cnt += snprintf(buf + cnt, max(sz - cnt, 0), "%d\n", a[i]);
If you have a series of multiple snprintf's, you can even leave off
checking that cnt goes past buf+sz on each snprintf call, and just once,
after the last snprintf, look for the -ENOMEM error case if cnt > sz.
Just don't call snprintf with a negative length - rather than doing what
I would prefer, which is writing nothing successfully, instead it
WARN_ON()'s one time of an out-of-range value: "Badness in func at
file:line" This is the reason for the max(), in the above example.
You could even do the same sort of logic with the "envp[i++] = buffer"
lines, changing them to:
if (i < num_envp)
envp[i++] = buffer;
Then all just once, at the end:
if (i == num_envp || length >= buffer_size)
return -ENOMEM;
This removes several return's in the middle of the procedure.
In other words, perhaps instead of encapsulating the ugliness in a macro
so as to avoid repeating it, try reducing it?
Something like the following, never before seen by a compiler, patch to
2.6.9-rc2-mm4:
diff -Naurp 2.6.9-rc2-mm4.orig/drivers/usb/core/usb.c 2.6.9-rc2-mm4/drivers/usb/core/usb.c
--- 2.6.9-rc2-mm4.orig/drivers/usb/core/usb.c 2004-09-27 22:59:01.382907032 -0700
+++ 2.6.9-rc2-mm4/drivers/usb/core/usb.c 2004-09-27 23:40:03.968537112 -0700
@@ -582,7 +582,6 @@ static int usb_hotplug (struct device *d
{
struct usb_interface *intf;
struct usb_device *usb_dev;
- char *scratch;
int i = 0;
int length = 0;
@@ -609,8 +608,6 @@ static int usb_hotplug (struct device *d
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
@@ -618,37 +615,30 @@ static int usb_hotplug (struct device *d
*
* FIXME reduce hardwired intelligence here
*/
- envp [i++] = scratch;
- length += snprintf (scratch, buffer_size - length,
+ if (i < num_envp)
+ envp [i++] = buffer + length;
+ length += snprintf (buffer + length, max(buffer_size - length, 0),
"DEVICE=/proc/bus/usb/%03d/%03d",
usb_dev->bus->busnum, usb_dev->devnum);
- if ((buffer_size - length <= 0) || (i >= num_envp))
- 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 (i < num_envp)
+ envp [i++] = buffer + length;
+ length += snprintf (buffer + length, max(buffer_size - length, 0),
+ "PRODUCT=%x/%x/%x",
usb_dev->descriptor.idVendor,
usb_dev->descriptor.idProduct,
usb_dev->descriptor.bcdDevice);
- if ((buffer_size - length <= 0) || (i >= num_envp))
- return -ENOMEM;
- ++length;
- scratch += length;
/* class-based driver binding models */
- envp [i++] = scratch;
- length += snprintf (scratch, buffer_size - length, "TYPE=%d/%d/%d",
+ if (i < num_envp)
+ envp [i++] = buffer + length;
+ length += snprintf (buffer + length, max(buffer_size - length, 0),
+ "TYPE=%d/%d/%d",
usb_dev->descriptor.bDeviceClass,
usb_dev->descriptor.bDeviceSubClass,
usb_dev->descriptor.bDeviceProtocol);
- if ((buffer_size - length <= 0) || (i >= num_envp))
- return -ENOMEM;
- ++length;
- scratch += length;
if (usb_dev->descriptor.bDeviceClass == 0) {
struct usb_host_interface *alt = intf->cur_altsetting;
@@ -657,19 +647,19 @@ static int usb_hotplug (struct device *d
* agents are called for all interfaces, and can use
* $DEVPATH/bInterfaceNumber if necessary.
*/
- envp [i++] = scratch;
- length += snprintf (scratch, buffer_size - length,
+ if (i < num_envp)
+ envp [i++] = buffer + length;
+ length += snprintf (buffer + length,
+ max(buffer_size - length, 0),
"INTERFACE=%d/%d/%d",
alt->desc.bInterfaceClass,
alt->desc.bInterfaceSubClass,
alt->desc.bInterfaceProtocol);
- if ((buffer_size - length <= 0) || (i >= num_envp))
- return -ENOMEM;
- ++length;
- scratch += length;
-
}
- envp[i++] = NULL;
+ if (i < num_envp)
+ envp[i++] = NULL;
+ if (i == num_envp || length >= buffer_size)
+ return -ENOMEM;
return 0;
}
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.650.933.1373
Your patch is buggy:
+ length += snprintf (buffer + length, max(buffer_size - length, 0),
"DEVICE=/proc/bus/usb/%03d/%03d",
usb_dev->bus->busnum, usb_dev->devnum);
snprintf() returns the number of characters that would be written not
counting the trailing NUL. So the next env var is going to be
concatenated with this one.
It's precisely this sort of easy-to-make off-by-one bug that convinces
me the hotplug environment variable handling needs to be wrapped up in
a helper macro or function.
Thanks,
Roland
> So the next env var is going to be concatenated with this one.
Right you are - unlike most times where one is trying to concatenate
strings, this time you want the nul char separator.
> It's precisely this sort of easy-to-make off-by-one bug that convinces
> me the hotplug environment variable handling needs to be wrapped up in
> a helper macro or function.
Perhaps - but perhaps also I've shown you ways to use a function with
fewer non-const variables.
And perhaps the placing of the nul chars should be explicit:
if (length < buffer_size)
buffer[length++] = '\0';
so that someone else doesn't either miss the intentional inclusion of
the nul as I just did, or does see it and thinks it's an off-by-one
error and "fixes" it.
That macro was ugly.
Adding to my previous rules of:
* minimum number variables
* simplest invariants on variables
* functions beat macros
now include:
* explicit coding of anything out of the ordinary.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.650.933.1373
Paul> Perhaps - but perhaps also I've shown you ways to use a
Paul> function with fewer non-const variables.
Yeah, you've convinced me. I'll reroll my patches.
Thanks,
Roland
On Tue, Sep 28, 2004 at 09:13:17AM -0700, Roland Dreier wrote:
> Paul> Perhaps - but perhaps also I've shown you ways to use a
> Paul> function with fewer non-const variables.
>
> Yeah, you've convinced me. I'll reroll my patches.
Ok, I've thrown away your old ones :)
thanks,
greg k-h