Date: Mon, 23 Aug 2004 11:41:24 -0400 From: John Baldwin <jhb@FreeBSD.org> To: freebsd-arch@FreeBSD.org Cc: Giorgos Keramidas <keramida@ceid.upatras.gr> Subject: Re: Introducing a poweroff(8) command Message-ID: <200408231141.25077.jhb@FreeBSD.org> In-Reply-To: <20040821202252.GB94336@gothmog.gr> References: <20040821191659.GA94336@gothmog.gr> <20040821202252.GB94336@gothmog.gr>
next in thread | previous in thread | raw e-mail | index | archive | help
On Saturday 21 August 2004 04:22 pm, Giorgos Keramidas wrote:
> On 2004-08-21 22:16, Giorgos Keramidas <keramida@freebsd.org> wrote:
> > Hi guys,
> >
> > In response to PR misc/70476 about `halt -p' I wrote a simple patch to
> > introduce a "poweroff" command that will default to "halt -p" behavior
> > and inhibit the need for changing the default behavior of halt(8).
> >
> > o Does this look ok to you all?
> >
> > o Should I suggest using it as a workaround of the behavior
> > described in the PR?
>
> It would be nice if I also included the patch :-?
>
> %%%
> Add a hard link to reboot(8) called "poweroff" which defaults to the
> same behavior as "halt -p". Also fix a related bug while here. When
> called as "halt -p" the previous reboot program would not disallow the
> use of -d for saving kernel dumps (it would inhibit dumps without the
> -p option though).
>
> Index: Makefile
> ===================================================================
> RCS file: /home/ncvs/src/sbin/reboot/Makefile,v
> retrieving revision 1.13
> diff -u -r1.13 Makefile
> --- Makefile 22 Mar 2004 00:52:27 -0000 1.13
> +++ Makefile 21 Aug 2004 18:45:36 -0000
> @@ -14,8 +14,8 @@
> MLINKS+= boot_i386.8 boot.8
> .endif
>
> -LINKS= ${BINDIR}/reboot ${BINDIR}/halt ${BINDIR}/reboot ${BINDIR}/fastboot
> \ - ${BINDIR}/reboot ${BINDIR}/fasthalt
> +LINKS= ${BINDIR}/reboot ${BINDIR}/halt ${BINDIR}/reboot ${BINDIR}/poweroff
> \ + ${BINDIR}/reboot ${BINDIR}/fastboot ${BINDIR}/reboot ${BINDIR}/fasthalt
>
> SCRIPTS= nextboot.sh
>
> Index: reboot.8
> ===================================================================
> RCS file: /home/ncvs/src/sbin/reboot/reboot.8,v
> retrieving revision 1.22
> diff -u -r1.22 reboot.8
> --- reboot.8 9 Apr 2004 19:58:35 -0000 1.22
> +++ reboot.8 17 Aug 2004 22:04:34 -0000
> @@ -34,6 +34,7 @@
> .Sh NAME
> .Nm reboot ,
> .Nm halt ,
> +.Nm poweroff ,
> .Nm fastboot ,
> .Nm fasthalt
> .Nd stopping and restarting the system
> @@ -41,6 +42,9 @@
> .Nm halt
> .Op Fl lnqp
> .Op Fl k Ar kernel
> +.Nm poweroff
> +.Op Fl lnqp
> +.Op Fl k Ar kernel
> .Nm
> .Op Fl dlnqp
> .Op Fl k Ar kernel
> @@ -52,7 +56,8 @@
> .Op Fl k Ar kernel
> .Sh DESCRIPTION
> The
> -.Nm halt
> +.Nm halt ,
> +.Nm poweroff
> and
> .Nm
> utilities flush the file system cache to disk, send all running processes
> @@ -91,9 +96,10 @@
> This option is intended for applications such as
> .Xr shutdown 8 ,
> that call
> -.Nm
> -or
> +.Nm ,
> .Nm halt
> +or
> +.Nm poweroff
> and log this themselves.
> .It Fl n
> The file system cache is not flushed.
> @@ -106,6 +112,9 @@
> This option should probably not be used.
> .It Fl p
> The system will turn off the power if it can.
> +This is the default action if
> +.Nm poweroff
> +is called.
> If the power down action fails, the system
> will halt or reboot normally, depending on whether
> .Nm halt
> Index: reboot.c
> ===================================================================
> RCS file: /home/ncvs/src/sbin/reboot/reboot.c,v
> retrieving revision 1.20
> diff -u -r1.20 reboot.c
> --- reboot.c 9 Apr 2004 19:58:35 -0000 1.20
> +++ reboot.c 17 Aug 2004 21:55:57 -0000
> @@ -70,9 +70,13 @@
> char *kernel, *p;
> const char *user;
>
> - if (strstr((p = rindex(*argv, '/')) ? p + 1 : *argv, "halt")) {
> + p = rindex(*argv, '/') ? p + 1 : *argv;
> + if (strcmp(p, "halt") == 0) {
I think this is buggy in that p will point to the / character since you don't
modify it in the second case. I.e. what you wrote is basically this:
p = rindex(*argv, '/');
if (p != NULL)
p + 1; /* does nothing */
else
*argv; /* also does nothing */
Note that in the old code the expression generates a char * that gets passed
to strstr(), but in your version the result of the expression is just
discarded. gcc(1) should have warned about this. (Assuming you compiled
with warnings on of course. :)
It would probably be better to use an explicit if statement rather than the
more obfuscated ?: in this case anyways:
p = rindex(argv[0], '/');
if (p != NULL)
p++;
else
p = argv[0];
Also, given the behavior of poweroff(8) on other OS's, I think that it would
violate POLA to make it a link to shutdown. If a sysadmin wants to be nice
and/or wants to run rc.d scripts on shutdown he can already use 'shutdown
[-hrp]' as it is. The fact that shutdown does all 3 via command line options
but halt/reboot/poweroff each only do one I think it makes more sense for
poweroff to link to halt/reboot.
--
John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve" = http://www.FreeBSD.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200408231141.25077.jhb>