Skip to contentSkip to conversation

support arguments in callout exec

udev Arnd Bergmann 3 participants 10 messages Nov 20, 2003 – Nov 22, 2003

lore
Share & tools
lore

Arnd Bergmann

author
#0 lore raw

On Thursday 20 November 2003 13:31, Kay Sievers wrote:> +                       for (i=0; i < CALLOUT_MAXARG; i++) {> +                               args[i] = strsep(&arg, " ");> +                               if (args[i] = NULL)> +                                       break;> +                       }> +                       if (args[i])> +                               dbg("to many args"); This still doesn't look correct: args[i] will be out of boundswhen the loop has finished on CALLOUT_MAXARG, and the argsarray is not zero terminated when calling execve.
I haven't tried it yet, but I think this would make more sense:
+                       for (i=0; i < CALLOUT_MAXARG-1; i++) {+                               args[i] = strsep(&arg, " ");+                               if (args[i] = NULL)+                                       break;+                       }+ args[i] = arg;+                       if (args[i])+                               dbg("to many args");
Aside from that, it looks good. Maybe there should be a way to escapespaces in the argument in case someone wants to do something like that:
CALLOUT, PROGRAM="sh -c 'echo ${DEVPATH} | tr a-z A-Z'", BUS="usb", ID="XXX", NAME="webcam%n"
To take that even further, do you think it might be a good idea to preprocess the string in the same way as the NAME= argument, allowing e.g.PROGRAM="/bin/devname %b %M:%m" ?
Arnd <><ÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÓ†+ù޵隊X¬²š'²ŠÞu¼ÿH_çzÑ¢½æÚrCë¢Ú›ðèzĨº·Šà{ùÞ¶¥§*.m騭êk¡Ûœ¶+Þü:²+azZr¢ç+y«^mëmz·(uïÒDLq 9QÿjwazZn²¥¥ƒ”ü)brAÞ­ïá¶Úÿÿû(º·~Šà{ùÞ·÷h�«^ÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿ.)îÇøh¶™nƒ÷^½éfj)bž b²Øm¶ŸÿþX§»á¢Úeºì¢êÜyú+�ïçzÒâžìÿ†‹i–èÿuëÞ—ùb²Ûÿ²‹«qçè®ÿ�ëa¶ÚlÿÿåŠËlþÊ.­ÇŸ¢¸þw­þX¬¶ÏåŠËb�ú?–)îÇøh¶™nƒ÷^

author
#1 lore raw

On Thursday 20 November 2003 17:07, Kay Sievers wrote:> On Thu, Nov 20, 2003 at 01:47:36AM +0100, Arnd Bergmann wrote:> > This still doesn't look correct: args[i] will be out of bounds> > when the loop has finished on CALLOUT_MAXARG, and the args> > array is not zero terminated when calling execve.>> Good catch, but arg is not NULL if MAXARG is reached - so args is still> not terminated :)

Quoted 1 line

+       char *args[CALLOUT_MAXARG];> +       int i;...> +                       for (i=0; i < CALLOUT_MAXARG; i++) {> +                               args[i] = strsep(&arg, " ");> +                               if (args[i] = NULL)> +                                       break;> +                       }> +                       if (args[i]) {> +                               dbg("to many args");> +                               args[i] = NULL;> +                       }

Ok, it's terminated now, but again out of bounds. It should bechar *args[CALLOUT_MAXARG+1];in the beginning or only loop to (CALLOUT_MAXARG - 1).
Arnd <><ÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÓ†+ù޵隊X¬²š'²ŠÞu¼ÿH_çzÑ¢½æÚrCë¢Ú›ðèzĨº·Šà{ùÞ¶¥§*.m騭êk¡Ûœ¶+Þü:²+azZr¢ç+y«^mëmz·(uïÒDLq 9QÿjwazZn²¥¥ƒ”ü)brAÞ­ïá¶Úÿÿû(º·~Šà{ùÞ·÷h�«^ÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿ.)îÇøh¶™nƒ÷^½éfj)bž b²Øm¶ŸÿþX§»á¢Úeºì¢êÜyú+�ïçzÒâžìÿ†‹i–èÿuëÞ—ùb²Ûÿ²‹«qçè®ÿ�ëa¶ÚlÿÿåŠËlþÊ.­ÇŸ¢¸þw­þX¬¶ÏåŠËb�ú?–)îÇøh¶™nƒ÷^

#2 lore raw
In reply to Arnd Bergmann

Sorry, please ignore the previous version it breaks scsi_id by eating main_argv. Now, with no arguments are given, behavior isn't changed.

here is argument support for CALLOUT exec:

CALLOUT, PROGRAM="/bin/echo -n xxx", BUS="usb", ID="xxx", NAME="webcam%n"

results in:

Nov 20 02:35:20 pim udev[30422]: get_major_minor: found major = 81, minor = 0
Nov 20 02:35:20 pim udev[30422]: exec_callout: callout to /bin/echo -n xxx
Nov 20 02:35:20 pim udev[30422]: exec_callout: callout returned 'xxx'
Nov 20 02:35:20 pim udev[30422]: get_attr: kernel number appended: 0

The feature is really nice, but the maximum argument count is hard coded to 8. What do you think?

thanks,
Kay

09-namedev.c-callout-argument-support.diff
namdev.c - support for argument in CALLOUT exec

namedev.c renamed from udev/namedev.c +17 -1
@@ -480,6 +480,9 @@
 	pid_t pid;
 	int value_set = 0;
 	char buffer[256];
+	char *prog;
+	char *args[8];
+	int i;
 
 	dbg("callout to %s\n", dev->exec_program);
 	retval = pipe(fds);

@@ -499,7 +502,19 @@
 		 */
 		close(STDOUT_FILENO);
 		dup(fds[1]);	/* dup write side of pipe to STDOUT */
-		retval = execve(dev->exec_program, main_argv, main_envp);
+		if (strchr(dev->exec_program, ' ')) {
+			/* callout with arguments */
+			strcpy(buffer, dev->exec_program);
+			prog = strtok(buffer, " ");
+			for(i=1; i<=8; i++) {
+				args[i] = strtok(NULL, " ");
+				if (args[i] == NULL)
+					break;
+			}
+			retval = execve(prog, args, main_envp);
+		} else {
+			retval = execve(dev->exec_program, main_argv, main_envp);
+		}
 		if (retval != 0) {
 			dbg("child execve failed");
 			exit(1);

@@ -528,6 +543,7 @@
 				strncpy(value, buffer, len);
 			}
 		}
+		dbg("callout returned '%s'", value);
 		close(fds[0]);
 		res = wait(&status);
 		if (res < 0) {
#3 lore raw
In reply to Arnd Bergmann

Third round :)
Improved version, using strsep() instead of strtok().

here is argument support for CALLOUT exec:

CALLOUT, PROGRAM="/bin/echo -n xxx", BUS="usb", ID="xxx", NAME="webcam%n"

results in:

Nov 20 02:35:20 pim udev[30422]: get_major_minor: found major = 81, minor = 0
Nov 20 02:35:20 pim udev[30422]: exec_callout: callout to /bin/echo -n xxx
Nov 20 02:35:20 pim udev[30422]: exec_callout: callout returned 'xxx'
Nov 20 02:35:20 pim udev[30422]: get_attr: kernel number appended: 0

The feature is really nice, but the maximum argument count is hard coded to 8. What do you think?

thanks,
Kay

09-namedev.c-callout-argument-support.diff
namdev.c - support for argument in CALLOUT exec

namedev.c renamed from udev/namedev.c +20 -1
@@ -45,6 +45,7 @@
 #define TYPE_TOPOLOGY	"TOPOLOGY"
 #define TYPE_REPLACE	"REPLACE"
 #define TYPE_CALLOUT	"CALLOUT"
+#define CALLOUT_MAXARG	8
 
 static LIST_HEAD(config_device_list);
 

@@ -480,6 +481,9 @@
 	pid_t pid;
 	int value_set = 0;
 	char buffer[256];
+	char *arg;
+	char *args[CALLOUT_MAXARG];
+	int i;
 
 	dbg("callout to %s\n", dev->exec_program);
 	retval = pipe(fds);

@@ -499,7 +503,21 @@
 		 */
 		close(STDOUT_FILENO);
 		dup(fds[1]);	/* dup write side of pipe to STDOUT */
-		retval = execve(dev->exec_program, main_argv, main_envp);
+		if (strchr(dev->exec_program, ' ')) {
+			/* callout with arguments */
+			arg = dev->exec_program;
+			for (i=0; i < CALLOUT_MAXARG; i++) {
+				args[i] = strsep(&arg, " ");
+				if (args[i] == NULL)
+					break;
+			}
+			if (args[i])
+				dbg("to many args");
+
+			retval = execve(args[0], args, main_envp);
+		} else {
+			retval = execve(dev->exec_program, main_argv, main_envp);
+		}
 		if (retval != 0) {
 			dbg("child execve failed");
 			exit(1);

@@ -528,6 +546,7 @@
 				strncpy(value, buffer, len);
 			}
 		}
+		dbg("callout returned '%s'", value);
 		close(fds[0]);
 		res = wait(&status);
 		if (res < 0) {
#4 lore raw
In reply to Arnd Bergmann
Quoted 20 lines · Arnd Bergmann
Quoted 7 lines · Kay Sievers

+                       for (i=0; i < CALLOUT_MAXARG; i++) { +                               args[i] = strsep(&arg, " "); +                               if (args[i] == NULL) +                                       break;
+                       }
+                       if (args[i])
+                               dbg("to many args");

This still doesn't look correct: args[i] will be out of bounds when the loop has finished on CALLOUT_MAXARG, and the args array is not zero terminated when calling execve.

I haven't tried it yet, but I think this would make more sense:

+                       for (i=0; i < CALLOUT_MAXARG-1; i++) { +                               args[i] = strsep(&arg, " "); +                               if (args[i] == NULL) +                                       break;
+                       }

Review

Quoted 3 lines
(quoted diff) +3 -0
+			args[i] = arg;
+                       if (args[i])
+                               dbg("to many args");

Good catch, but arg is not NULL if MAXARG is reached - so args is still not terminated :)

Corrected patch attached.

Grüße
Kay

namedev.c renamed from udev/namedev.c +21 -1
@@ -45,6 +45,7 @@
 #define TYPE_TOPOLOGY	"TOPOLOGY"
 #define TYPE_REPLACE	"REPLACE"
 #define TYPE_CALLOUT	"CALLOUT"
+#define CALLOUT_MAXARG	8
 
 static LIST_HEAD(config_device_list);
 

@@ -480,6 +481,9 @@
 	pid_t pid;
 	int value_set = 0;
 	char buffer[256];
+	char *arg;
+	char *args[CALLOUT_MAXARG];
+	int i;
 
 	dbg("callout to %s\n", dev->exec_program);
 	retval = pipe(fds);

@@ -499,7 +503,22 @@
 		 */
 		close(STDOUT_FILENO);
 		dup(fds[1]);	/* dup write side of pipe to STDOUT */
-		retval = execve(dev->exec_program, main_argv, main_envp);
+		if (strchr(dev->exec_program, ' ')) {
+			/* callout with arguments */
+			arg = dev->exec_program;
+			for (i=0; i < CALLOUT_MAXARG; i++) {
+				args[i] = strsep(&arg, " ");
+				if (args[i] == NULL)
+					break;
+			}
+			if (args[i]) {
+				dbg("to many args");
+				args[i] = NULL;
+			}
+			retval = execve(args[0], args, main_envp);
+		} else {
+			retval = execve(dev->exec_program, main_argv, main_envp);
+		}
 		if (retval != 0) {
 			dbg("child execve failed");
 			exit(1);

@@ -528,6 +547,7 @@
 				strncpy(value, buffer, len);
 			}
 		}
+		dbg("callout returned '%s'", value);
 		close(fds[0]);
 		res = wait(&status);
 		if (res < 0) {
#5 lore raw
In reply to Arnd Bergmann
Quoted 22 lines · Arnd Bergmann
Quoted 10 lines · Kay Sievers
Quoted 3 lines · Arnd Bergmann

This still doesn't look correct: args[i] will be out of bounds when the loop has finished on CALLOUT_MAXARG, and the args array is not zero terminated when calling execve.

Good catch, but arg is not NULL if MAXARG is reached - so args is still not terminated :)

+       char *args[CALLOUT_MAXARG];
+       int i;

...

Quoted 9 lines

+                       for (i=0; i < CALLOUT_MAXARG; i++) { +                               args[i] = strsep(&arg, " "); +                               if (args[i] == NULL) +                                       break;
+                       }
+                       if (args[i]) {
+                               dbg("to many args"); +                               args[i] = NULL;
+                       }

Quoted 3 lines

Ok, it's terminated now, but again out of bounds. It should be char *args[CALLOUT_MAXARG+1];
in the beginning or only loop to (CALLOUT_MAXARG - 1).

Oh, oh, oh...
I hope, I don't send more patches than the number of lines added to the code. Here is #5 :)

thanks,
Kay

namedev.c renamed from udev/namedev.c +21 -1
@@ -45,6 +45,7 @@
 #define TYPE_TOPOLOGY	"TOPOLOGY"
 #define TYPE_REPLACE	"REPLACE"
 #define TYPE_CALLOUT	"CALLOUT"
+#define CALLOUT_MAXARG	8
 
 static LIST_HEAD(config_device_list);
 

@@ -480,6 +481,9 @@
 	pid_t pid;
 	int value_set = 0;
 	char buffer[256];
+	char *arg;
+	char *args[CALLOUT_MAXARG];
+	int i;
 
 	dbg("callout to %s\n", dev->exec_program);
 	retval = pipe(fds);

@@ -499,7 +503,22 @@
 		 */
 		close(STDOUT_FILENO);
 		dup(fds[1]);	/* dup write side of pipe to STDOUT */
-		retval = execve(dev->exec_program, main_argv, main_envp);
+		if (strchr(dev->exec_program, ' ')) {
+			/* callout with arguments */
+			arg = dev->exec_program;
+			for (i=0; i < CALLOUT_MAXARG-1; i++) {
+				args[i] = strsep(&arg, " ");
+				if (args[i] == NULL)
+					break;
+			}
+			if (args[i]) {
+				dbg("to many args - %d", i);
+				args[i] = NULL;
+			}
+			retval = execve(args[0], args, main_envp);
+		} else {
+			retval = execve(dev->exec_program, main_argv, main_envp);
+		}
 		if (retval != 0) {
 			dbg("child execve failed");
 			exit(1);

@@ -528,6 +547,7 @@
 				strncpy(value, buffer, len);
 			}
 		}
+		dbg("callout returned '%s'", value);
 		close(fds[0]);
 		res = wait(&status);
 		if (res < 0) {
author
#6 lore raw
In reply to Arnd Bergmann
Quoted 6 lines · Greg KH
Quoted 2 lines · Arnd Bergmann

CALLOUT, PROGRAM="sh -c 'echo ${DEVPATH} | tr a-z A-Z'", BUS="usb", ID="XXX", NAME="webcam%n"

Oh come on, why not just make that into a simple script, we're trying to keep udev simple :)

Ok, that makes sense.

Quoted 11 lines
Quoted 3 lines

To take that even further, do you think it might be a good idea to preprocess the string in the same way as the NAME= argument, allowing e.g. PROGRAM="/bin/devname %b %M:%m" ?

Hm, good idea.

Someone else also proposed adding a number modifier to the %b field to pick out a specific character from a bus id. Anyone have any thoughts about this? Personally, I think it will be a bit hard to handle bus ids that are not fixed widths all the time, but might be handy to try to implement a devfs-like udev config file.

When we allow %b in PROGRAM= and find a good way to use the output of the program as the device name, there won't be any need for such a hack, as it can better be done in an external script. Getting it right in udev would be too complicated IMHO (both usage and implementation).

	Arnd <><

-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program. Does SourceForge.net help you be more productive? Does it help you create better code? SHARE THE LOVE, and help us help YOU! Click Here: http://sourceforge.net/donate/
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

#7 lore raw
In reply to Arnd Bergmann
Quoted 3 lines · Kay Sievers

Oh, oh, oh...
I hope, I don't send more patches than the number of lines added to the code. Here is #5 :)

Heh, this looks good, I've applied it.

thanks,

greg k-h

-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program. Does SourceForge.net help you be more productive? Does it help you create better code? SHARE THE LOVE, and help us help YOU! Click Here: http://sourceforge.net/donate/
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

#8 lore raw
In reply to Arnd Bergmann
Quoted 5 lines · Arnd Bergmann

Aside from that, it looks good. Maybe there should be a way to escape spaces in the argument in case someone wants to do something like that:

CALLOUT, PROGRAM="sh -c 'echo ${DEVPATH} | tr a-z A-Z'", BUS="usb", ID="XXX", NAME="webcam%n"

Oh come on, why not just make that into a simple script, we're trying to keep udev simple :)

Quoted 3 lines

To take that even further, do you think it might be a good idea to preprocess the string in the same way as the NAME= argument, allowing e.g. PROGRAM="/bin/devname %b %M:%m" ?

Hm, good idea.

Someone else also proposed adding a number modifier to the %b field to pick out a specific character from a bus id. Anyone have any thoughts about this? Personally, I think it will be a bit hard to handle bus ids that are not fixed widths all the time, but might be handy to try to implement a devfs-like udev config file.

thanks,

greg k-h

-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program. Does SourceForge.net help you be more productive? Does it help you create better code? SHARE THE LOVE, and help us help YOU! Click Here: http://sourceforge.net/donate/
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

#9 lore raw
In reply to Arnd Bergmann
Quoted 12 lines · Arnd Bergmann
Quoted 11 lines
Quoted 3 lines

To take that even further, do you think it might be a good idea to preprocess the string in the same way as the NAME= argument, allowing e.g. PROGRAM="/bin/devname %b %M:%m" ?

Hm, good idea.

Someone else also proposed adding a number modifier to the %b field to pick out a specific character from a bus id. Anyone have any thoughts about this? Personally, I think it will be a bit hard to handle bus ids that are not fixed widths all the time, but might be handy to try to implement a devfs-like udev config file.

Quoted 4 lines

When we allow %b in PROGRAM= and find a good way to use the output of the program as the device name, there won't be any need for such a hack, as it can better be done in an external script. Getting it right in udev would be too complicated IMHO (both usage and implementation).

Good point, I'll try to work on adding that this weekend if I get a break from changing diapers...

greg k-h

-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program. Does SourceForge.net help you be more productive? Does it help you create better code? SHARE THE LOVE, and help us help YOU! Click Here: http://sourceforge.net/donate/
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel