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
--0000000000002ab542061184fcaa Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Feb 16, 2024 at 6:34=E2=80=AFAM Konstantin Belousov <kostikbel@gmai= l.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=3D33a2406eed009dbffd7dfa44285c23f= 0db5a3750 > > > > 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 =3D=3D NULL) > > - E("No memory for zfsbootcfg"); > > - > > - rv =3D system(k); > > - if (rv =3D=3D 0) > > - return; > > + const char * const av[] =3D { > 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 =3D posix_spawnp(&p, av[0], NULL, NULL, __DECONST(char **, av)= , > > + environ); > > if (rv =3D=3D -1) > > E("system zfsbootcfg"); > > - if (rv =3D=3D 127) > > - E("zfsbootcfg not found in path"); > > - E("zfsbootcfg returned %d", rv); > > + if (waitpid(p, &status, WEXITED) < 0) { > > + if (errno =3D=3D EINTR) > > + return; > > + E("waitpid zfsbootcfg"); > > + } > > + if (WIFEXITED(status)) { > > + int e =3D WEXITSTATUS(status); > > + > > + if (e =3D=3D 0) > > + return; > > + if (e =3D=3D 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 > --0000000000002ab542061184fcaa Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">= <div dir=3D"ltr" class=3D"gmail_attr">On Fri, Feb 16, 2024 at 6:34=E2=80=AF= AM Konstantin Belousov <<a href=3D"mailto:kostikbel@gmail.com">kostikbel= @gmail.com</a>> wrote:<br></div><blockquote class=3D"gmail_quote" style= =3D"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=3D"https://cgit.FreeBSD.org/src/commit/?id=3D33a2406eed00= 9dbffd7dfa44285c23f0db5a3750" rel=3D"noreferrer" target=3D"_blank">https://= cgit.FreeBSD.org/src/commit/?id=3D33a2406eed009dbffd7dfa44285c23f0db5a3750<= /a><br> > <br> > commit 33a2406eed009dbffd7dfa44285c23f0db5a3750<br> > Author:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org><br> > AuthorDate: 2024-02-16 03:52:31 +0000<br> > Commit:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org><br> > CommitDate: 2024-02-16 03:59:22 +0000<br> > <br> >=C2=A0 =C2=A0 =C2=A0reboot: Use posix_spawn instead of system<br> >=C2=A0 =C2=A0 =C2=A0<br> >=C2=A0 =C2=A0 =C2=A0Use posix_spawn to avoid having to allocate memory = needed for the system<br> >=C2=A0 =C2=A0 =C2=A0command line.<br> >=C2=A0 =C2=A0 =C2=A0<br> >=C2=A0 =C2=A0 =C2=A0Sponsored by:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0Netflix<br> >=C2=A0 =C2=A0 =C2=A0Reviewed by:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 jrtc27<br> >=C2=A0 =C2=A0 =C2=A0Differential Revision:=C2=A0 <a href=3D"https://rev= iews.freebsd.org/D43860" rel=3D"noreferrer" target=3D"_blank">https://revie= ws.freebsd.org/D43860</a><br> > ---<br> >=C2=A0 sbin/reboot/reboot.c | 54 ++++++++++++++++++++++++++++++++++++--= --------------<br> >=C2=A0 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> >=C2=A0 =C2=A0* SUCH DAMAGE.<br> >=C2=A0 =C2=A0*/<br> >=C2=A0 <br> > -#include <sys/types.h><br> >=C2=A0 #include <sys/boottrace.h><br> >=C2=A0 #include <sys/mount.h><br> >=C2=A0 #include <sys/reboot.h><br> >=C2=A0 #include <sys/stat.h><br> >=C2=A0 #include <sys/sysctl.h><br> >=C2=A0 #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></blockquot= e><div><br></div><div>Yea, it's not needed at all, so I'm removing = it.<br></div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"ma= rgin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:= 1ex"> >=C2=A0 <br> >=C2=A0 #include <err.h><br> >=C2=A0 #include <errno.h><br> >=C2=A0 #include <fcntl.h><br> >=C2=A0 #include <pwd.h><br> >=C2=A0 #include <signal.h><br> > +#include <spawn.h><br> >=C2=A0 #include <stdbool.h><br> >=C2=A0 #include <stdio.h><br> >=C2=A0 #include <stdlib.h><br> > @@ -69,23 +71,43 @@ static bool donextboot;<br> >=C2=A0 static void<br> >=C2=A0 zfsbootcfg(const char *pool, bool force)<br> >=C2=A0 {<br> > -=C2=A0 =C2=A0 =C2=A0char *k;<br> > -=C2=A0 =C2=A0 =C2=A0int rv;<br> > -<br> > -=C2=A0 =C2=A0 =C2=A0asprintf(&k,<br> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"zfsbootcfg -z %s -n freebsd:n= vstore -k nextboot_enable -v YES",<br> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pool);<br> > -=C2=A0 =C2=A0 =C2=A0if (k =3D=3D NULL)<br> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0E("No memory for= zfsbootcfg");<br> > -<br> > -=C2=A0 =C2=A0 =C2=A0rv =3D system(k);<br> > -=C2=A0 =C2=A0 =C2=A0if (rv =3D=3D 0)<br> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return;<br> > +=C2=A0 =C2=A0 =C2=A0const char * const av[] =3D {<br> Why not 'static'?<br> <br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"zfsbootcfg"= ;,<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"-z",<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pool,<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"-n",<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"freebsd:nvstore= ",<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"-k",<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"nextboot_enable= "<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"-v",<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"YES",<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0NULL<br> > +=C2=A0 =C2=A0 =C2=A0};<br></blockquote><div><br></div><div>Because &#= 39;pool' is not a compile time constant?</div><div><br></div><div>Warne= r<br></div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"marg= in:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1e= x"> > +=C2=A0 =C2=A0 =C2=A0int rv, status;<br> > +=C2=A0 =C2=A0 =C2=A0pid_t p;<br> > +=C2=A0 =C2=A0 =C2=A0extern char **environ;<br> > +<br> > +=C2=A0 =C2=A0 =C2=A0rv =3D posix_spawnp(&p, av[0], NULL, NULL, __= DECONST(char **, av),<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0environ);<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0if (rv =3D=3D -1)<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0E("system z= fsbootcfg");<br> > -=C2=A0 =C2=A0 =C2=A0if (rv =3D=3D 127)<br> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0E("zfsbootcfg no= t found in path");<br> > -=C2=A0 =C2=A0 =C2=A0E("zfsbootcfg returned %d", rv);<br> > +=C2=A0 =C2=A0 =C2=A0if (waitpid(p, &status, WEXITED) < 0) {<br= > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (errno =3D=3D EINT= R)<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return;<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0E("waitpid zfsbo= otcfg");<br> > +=C2=A0 =C2=A0 =C2=A0}<br> > +=C2=A0 =C2=A0 =C2=A0if (WIFEXITED(status)) {<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int e =3D WEXITSTATUS= (status);<br> > +<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (e =3D=3D 0)<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return;<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (e =3D=3D 127)<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0E("zfsbootcfg not found in path");<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0E("zfsbootcfg re= turned %d", e);<br> > +=C2=A0 =C2=A0 =C2=A0}<br> > +=C2=A0 =C2=A0 =C2=A0if (WIFSIGNALED(status))<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0E("zfsbootcfg di= ed with signal %d", WTERMSIG(status));<br> > +=C2=A0 =C2=A0 =C2=A0E("zfsbootcfg unexpected status %d", st= atus);<br> >=C2=A0 }<br> >=C2=A0 <br> >=C2=A0 static void<br> </blockquote></div></div> --0000000000002ab542061184fcaa--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfp9HisN83yF2e07XifjbZfjprUTO%2BkOD=tQX%2BU4fFO5SQ>