Re: git: 33a2406eed00 - main - reboot: Use posix_spawn instead of system

On Fri, Feb 16, 2024 at 04:00:19AM +0000, Warner Losh wrote:
> The branch main has been updated by imp:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=33a2406eed009dbffd7dfa44285c23f0db5a3750
> 
> commit 33a2406eed009dbffd7dfa44285c23f0db5a3750
> Author:     Warner Losh <imp@FreeBSD.org>
> AuthorDate: 2024-02-16 03:52:31 +0000
> Commit:     Warner Losh <imp@FreeBSD.org>
> CommitDate: 2024-02-16 03:59:22 +0000
> 
>     reboot: Use posix_spawn instead of system
>     
>     Use posix_spawn to avoid having to allocate memory needed for the system
>     command line.
>     
>     Sponsored by:           Netflix
>     Reviewed by:            jrtc27
>     Differential Revision:  https://reviews.freebsd.org/D43860
> ---
>  sbin/reboot/reboot.c | 54 ++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/sbin/reboot/reboot.c b/sbin/reboot/reboot.c
> index e9d6487da6a5..d3f1fc9bbb86 100644
> --- a/sbin/reboot/reboot.c
> +++ b/sbin/reboot/reboot.c
> @@ -29,19 +29,21 @@
>   * SUCH DAMAGE.
>   */
>  
> -#include <sys/types.h>
>  #include <sys/boottrace.h>
>  #include <sys/mount.h>
>  #include <sys/reboot.h>
>  #include <sys/stat.h>
>  #include <sys/sysctl.h>
>  #include <sys/time.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
sys/types.h should go first, the previous location was right

>  
>  #include <err.h>
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <pwd.h>
>  #include <signal.h>
> +#include <spawn.h>
>  #include <stdbool.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -69,23 +71,43 @@ static bool donextboot;
>  static void
>  zfsbootcfg(const char *pool, bool force)
>  {
> -	char *k;
> -	int rv;
> -
> -	asprintf(&k,
> -	    "zfsbootcfg -z %s -n freebsd:nvstore -k nextboot_enable -v YES",
> -	    pool);
> -	if (k == NULL)
> -		E("No memory for zfsbootcfg");
> -
> -	rv = system(k);
> -	if (rv == 0)
> -		return;
> +	const char * const av[] = {
Why not 'static'?

> +		"zfsbootcfg",
> +		"-z",
> +		pool,
> +		"-n",
> +		"freebsd:nvstore",
> +		"-k",
> +		"nextboot_enable"
> +		"-v",
> +		"YES",
> +		NULL
> +	};
> +	int rv, status;
> +	pid_t p;
> +	extern char **environ;
> +
> +	rv = posix_spawnp(&p, av[0], NULL, NULL, __DECONST(char **, av),
> +	    environ);
>  	if (rv == -1)
>  		E("system zfsbootcfg");
> -	if (rv == 127)
> -		E("zfsbootcfg not found in path");
> -	E("zfsbootcfg returned %d", rv);
> +	if (waitpid(p, &status, WEXITED) < 0) {
> +		if (errno == EINTR)
> +			return;
> +		E("waitpid zfsbootcfg");
> +	}
> +	if (WIFEXITED(status)) {
> +		int e = WEXITSTATUS(status);
> +
> +		if (e == 0)
> +			return;
> +		if (e == 127)
> +			E("zfsbootcfg not found in path");
> +		E("zfsbootcfg returned %d", e);
> +	}
> +	if (WIFSIGNALED(status))
> +		E("zfsbootcfg died with signal %d", WTERMSIG(status));
> +	E("zfsbootcfg unexpected status %d", status);
>  }
>  
>  static void