Date: Fri, 16 Feb 2024 12:45:14 -0700 From: Warner Losh <imp@bsdimp.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Warner Losh <imp@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 33a2406eed00 - main - reboot: Use posix_spawn instead of system Message-ID: <CANCZdfp9HisN83yF2e07XifjbZfjprUTO%2BkOD=tQX%2BU4fFO5SQ@mail.gmail.com> In-Reply-To: <Zc9kFvcR-h3G_95H@kib.kiev.ua> References: <202402160400.41G40Jgd018717@gitrepo.freebsd.org> <Zc9kFvcR-h3G_95H@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] 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 > [-- Attachment #2 --] <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Feb 16, 2024 at 6:34 AM Konstantin Belousov <<a href="mailto:kostikbel@gmail.com">kostikbel@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Feb 16, 2024 at 04:00:19AM +0000, Warner Losh wrote:<br> > The branch main has been updated by imp:<br> > <br> > URL: <a href="https://cgit.FreeBSD.org/src/commit/?id=33a2406eed009dbffd7dfa44285c23f0db5a3750" rel="noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=33a2406eed009dbffd7dfa44285c23f0db5a3750</a><br>; > <br> > commit 33a2406eed009dbffd7dfa44285c23f0db5a3750<br> > Author: Warner Losh <imp@FreeBSD.org><br> > AuthorDate: 2024-02-16 03:52:31 +0000<br> > Commit: Warner Losh <imp@FreeBSD.org><br> > CommitDate: 2024-02-16 03:59:22 +0000<br> > <br> > reboot: Use posix_spawn instead of system<br> > <br> > Use posix_spawn to avoid having to allocate memory needed for the system<br> > command line.<br> > <br> > Sponsored by: Netflix<br> > Reviewed by: jrtc27<br> > Differential Revision: <a href="https://reviews.freebsd.org/D43860" rel="noreferrer" target="_blank">https://reviews.freebsd.org/D43860</a><br>; > ---<br> > sbin/reboot/reboot.c | 54 ++++++++++++++++++++++++++++++++++++----------------<br> > 1 file changed, 38 insertions(+), 16 deletions(-)<br> > <br> > diff --git a/sbin/reboot/reboot.c b/sbin/reboot/reboot.c<br> > index e9d6487da6a5..d3f1fc9bbb86 100644<br> > --- a/sbin/reboot/reboot.c<br> > +++ b/sbin/reboot/reboot.c<br> > @@ -29,19 +29,21 @@<br> > * SUCH DAMAGE.<br> > */<br> > <br> > -#include <sys/types.h><br> > #include <sys/boottrace.h><br> > #include <sys/mount.h><br> > #include <sys/reboot.h><br> > #include <sys/stat.h><br> > #include <sys/sysctl.h><br> > #include <sys/time.h><br> > +#include <sys/types.h><br> > +#include <sys/wait.h><br> sys/types.h should go first, the previous location was right<br></blockquote><div><br></div><div>Yea, it's not needed at all, so I'm removing it.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > <br> > #include <err.h><br> > #include <errno.h><br> > #include <fcntl.h><br> > #include <pwd.h><br> > #include <signal.h><br> > +#include <spawn.h><br> > #include <stdbool.h><br> > #include <stdio.h><br> > #include <stdlib.h><br> > @@ -69,23 +71,43 @@ static bool donextboot;<br> > static void<br> > zfsbootcfg(const char *pool, bool force)<br> > {<br> > - char *k;<br> > - int rv;<br> > -<br> > - asprintf(&k,<br> > - "zfsbootcfg -z %s -n freebsd:nvstore -k nextboot_enable -v YES",<br> > - pool);<br> > - if (k == NULL)<br> > - E("No memory for zfsbootcfg");<br> > -<br> > - rv = system(k);<br> > - if (rv == 0)<br> > - return;<br> > + const char * const av[] = {<br> Why not 'static'?<br> <br> > + "zfsbootcfg",<br> > + "-z",<br> > + pool,<br> > + "-n",<br> > + "freebsd:nvstore",<br> > + "-k",<br> > + "nextboot_enable"<br> > + "-v",<br> > + "YES",<br> > + NULL<br> > + };<br></blockquote><div><br></div><div>Because 'pool' is not a compile time constant?</div><div><br></div><div>Warner<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + int rv, status;<br> > + pid_t p;<br> > + extern char **environ;<br> > +<br> > + rv = posix_spawnp(&p, av[0], NULL, NULL, __DECONST(char **, av),<br> > + environ);<br> > if (rv == -1)<br> > E("system zfsbootcfg");<br> > - if (rv == 127)<br> > - E("zfsbootcfg not found in path");<br> > - E("zfsbootcfg returned %d", rv);<br> > + if (waitpid(p, &status, WEXITED) < 0) {<br> > + if (errno == EINTR)<br> > + return;<br> > + E("waitpid zfsbootcfg");<br> > + }<br> > + if (WIFEXITED(status)) {<br> > + int e = WEXITSTATUS(status);<br> > +<br> > + if (e == 0)<br> > + return;<br> > + if (e == 127)<br> > + E("zfsbootcfg not found in path");<br> > + E("zfsbootcfg returned %d", e);<br> > + }<br> > + if (WIFSIGNALED(status))<br> > + E("zfsbootcfg died with signal %d", WTERMSIG(status));<br> > + E("zfsbootcfg unexpected status %d", status);<br> > }<br> > <br> > static void<br> </blockquote></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfp9HisN83yF2e07XifjbZfjprUTO%2BkOD=tQX%2BU4fFO5SQ>