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

On Fri, Feb 16, 2024 at 6:34 AM Konstantin Belousov <kostikbel@gmail.com>
wrote:

> 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
>

Yea, it's not needed at all, so I'm removing it.


> >
> >  #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
> > +     };
>

Because 'pool' is not a compile time constant?

Warner


> > +     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
>