Re: patch: use spawnve() for simple commands if available

[Eric Blake]

Keith Reynolds <keithr <at> keithr.org> writes:

> 
> Description:
>         The attached patch uses spawnve() for simple commands (no redirection 
or fds to close).
>         On cygwin, where fork() is notoriously slow, this patch cuts the 
execution time of
>         bash's own configure script from 5:09 to 2:55 on my 1.7GHz P4, almost 
exactly twice as fast.

The patch as written is wrong in a couple of places; see my comments below.  
Among other symptoms, after a call to spawn_child() completes on cygwin, the 
SIGINT handler is lost, such that Ctrl-C exits the shell instead of cancelling 
the current line.  I don't know if I can find where the interaction is going 
wrong, but until it is solved (whether your spawn_child() is buggy or cygwin's 
spawnve() is doing something weird to signals), this patch is not ready for 
primetime.

> +++ bash-3.0/execute_cmd.c    2005-07-16 00:34:51.692799100 -0700
>  <at>  <at>  -3499,67 +3499,96  <at>  <at>  setup_async_signals ()
>     6) If the execve failed, see if the file has executable mode set.
>     If so, and it isn't a directory, then execute its contents as
>     a shell script.
> 
>     Note that the filename hashing stuff has to take place up here,
>     in the parent.  This is probably why the Bourne style shells
>     don't handle it, since that would require them to go through
>     this gnarly hair, for no good reason.
> 
>     NOTE: callers expect this to fork or exit(). */
> -static void
> +static int

You are violating the assumptions stated in the comment, by returning without 
forking or calling exit.  That may be okay (because the comment may be out-of-
date), but then the comment should be updated to reflect the new reality of 
this method.  Or the violation of the comment may be the reason that you are 
losing the SIGINT handler.

> +++ bash-3.0/jobs.c   2005-07-16 00:31:56.098556100 -0700
>  <at>  <at>  -1433,20 +1437,55  <at>  <at>  make_child (command, async_p)
> 
>        last_made_pid = pid;
> 
>        /* Unblock SIGINT and SIGCHLD. */
>        sigprocmask (SIG_SETMASK, &oset, (sigset_t *)NULL);
>      }
> 
>    return (pid);
>  }
> 
> +#if defined (HAVE_SPAWNVE)
> +int
> +spawn_child (command, args, export_env)

Why are you sticking spawn_child in jobs.c, when it is only called when job 
control is off?  Isn't execute_cmd.c a better location, alongside shell_execve?

> +     char *command;
> +     char **args;
> +     char **export_env;
> +{
> +  sigset_t child_sig_mask;
> +  sigset_t old_sig_mask;
> +  pid_t pid;
> +  int result = EXECUTION_SUCCESS;
> +  int i;
> +
> +  /* Restore top-level signal mask, with job control
> +   * signals removed. */
> +  child_sig_mask = top_level_mask;
> +  sigdelset(&child_sig_mask, SIGTSTP);

You are assuming that HAVE_SPAWNVE implies HAVE_POSIX_SIGNALS, which is not 
necessarily the case.

> +  sigdelset(&child_sig_mask, SIGTTIN);
> +  sigdelset(&child_sig_mask, SIGTTOU);
> +  sigemptyset(&old_sig_mask);
> +  sigprocmask (SIG_SETMASK, &top_level_mask, &old_sig_mask);

Why are you setting the mask to top_level_mask, instead of child_sig_mask?

> +  result = spawnve(_P_WAIT, command, (const char * const *) args,
> +          (const char * const *) export_env);

At least on cygwin, spawnve returns a WAIT on success (ie. it does something 
like "if (wait (&result) != spawned_child) result = -1; return result").  This 
means you should probably use "result = process_exit_status (result)" if there 
was no error.  Otherwise, you get the wrong values (for example, on cygwin, $? 
after /bin/false should be 1, but with your patch is 256).  Since spawnve is 
non-standardized, it may require some more autoconf magic to decide how to 
correctly interpret the return value of spawnve on other platforms.

> +  i = errno;
> +  sigprocmask (SIG_SETMASK, &old_sig_mask, (sigset_t *)NULL);
> +  if (result == -1 && i != 0)
> +    {
> +      /* XXX Posix.2 says that exit status is 126 */
> +      result = ((i == ENOENT) ? EX_NOTFOUND : EX_NOEXEC);
> +    }

You didn't do half as much failure path checking as shell_execve, such as 
fallbacks if !HAVE_HASH_BANG_EXEC.  Could this ever be a problem?

> +
> +  return (result);
> +}
> +#endif /* HAVE_SPAWNVE */
> +

--
Eric Blake