Re: [Superkb-devel] [PATCH] Change system() to execvp() for command execution.

[Octavio A. <alv...@al...>]

On Fri, 10 Dec 2010 20:05:45 -0800, Eduardo A. Bustamante López  
<ebu...@du...> wrote:

> The system() call disables the calling process until the command
> is executed. This behavior generated a superkb child process
> for each command executed. With execvp(), the child process is
> replaced with the user's command.

Nice. I have some comments, though.

> @@ -279,6 +279,10 @@ int kbwin_init(Display * dpy)
>  void __Superkb_Action(KeyCode keycode, unsigned int state)
>  {
>  	unsigned int i;
> +	char *argv [4];

Drop the space between *argv and [4], please.

> +	argv[0] = "/bin/sh";
> +	argv[1] = "-c";
> +	argv[3] = NULL;

You might want to try char *argv[4] = { "/bin/sh", "-c", NULL, NULL }; it  
may be easier to optimize by the compiler.

Also, being execvp(), why not use just sh instead of /bin/sh?

> -					system(config->key_bindings[i].action.command);
> +					argv[2] = config->key_bindings[i].action.command;
> +					execvp(*argv, argv);
>  					exit(EXIT_SUCCESS);

We want exit(EXIT_FAILURE) here.

"If any of the exec functions returns, an error will have occurred."

> @@ -308,7 +314,8 @@ void __Superkb_Action(KeyCode keycode, unsigned int  
> state)
>  							strcat (cmdline, " ");
>  							strcat (cmdline, config->key_bindings[i].feedback_string);
>  							strcat (cmdline, " &");
> -							system(cmdline);
> +							argv[2] = cmdline;
> +							execvp(*argv, argv);
>  						}
>  					}
>  					char *cmdline = malloc(strlen(config->document_handler) +  
> strlen(config->key_bindings[i].action.document) + 2);

I'm not sure we want this. If you replace Superkb with the process for  
feedback_string, the execvp() below will never be reached.

> @@ -316,7 +323,8 @@ void __Superkb_Action(KeyCode keycode, unsigned int  
> state)
>  						strcpy (cmdline, config->document_handler);
>  						strcat (cmdline, " ");
>  						strcat (cmdline, config->key_bindings[i].action.document);
> -						system(cmdline);
> +						argv[2] = cmdline;
> +						execvp(*argv, argv);
>  					}
>  					exit(EXIT_SUCCESS);
>  				}

This one. We also want exit(EXIT_FAILURE) here.