[PATCH] halt: add option to call telinit when using external init

[Denys Vlasenko]

On Sunday 26 July 2009 14:45, Alexander Shishkin wrote:
> 2009/7/26 Denys Vlasenko <vda.linux at googlemail.com>:
> > On Saturday 25 July 2009 11:07, Alexander Shishkin wrote:
> >> Come to think of it, 9/10 of busybox functionality could be
> >> implemented in shell,
> >
> > And some of it definitely should have been. ifup is a candidate #1.
> > reboot also can be. An applet which does nothing else than starts
> > or signals other processes and neither runs for a long time (server)
> > nor contains complex logic, isn't really something which
> > is best written in C.
>
> I completely miss the point here. If it is already implemented in C,
> working and maintainable, why would anyone want to replace it with
> something scripted?

I don't want to replace anything.
I do not plan to remove ifup or reboot. I already said it.

> If I have to reboot and especially "reboot -f", 
> I'm very likely to be on a tight schedule and not have time to fork
> off a shell and have it interpret a script. There are also context
> switches, which are costly on some architectures.

How do you think init performs reboot? It usually
runs a shell script.

> Same stands for 
> ifup, which, be it a shell script will eat into my system's bootup
> time where every millisecond is valuable (I'm not talking about
> desktop machines).

Shell is not slow. ifup forking and execing things can't be
many times faster, since it is basically the same operations
shell script does. But ifup is many times *less flexible* than
a shell script, and for ifup it's a big disadvantage.

> > I do not plan to remove such applets, but will need a very good
> > rationale why it's good to add _more_ things like these.
>
> Because one might want things to be small and fast when one considers
> using busybox.

...but uses SysV init. Strange combination.

> > Technically speaking, the situation is like this:
> >
> > bbox reboot currently is meant to work with busybox init.
>
> I don't know where this comes from, but _technically_ (considering
> init/Config.in and the actual code) nothing says that.
> And even should that be the case, I don't see the point.

It is so because bbox reboot communicates with init in bbox-specific
way (signals) rather than some socket in /dev.

> > You propose to make it possible to use it with SysV init
> > (and any other kinds which have telinit), right?
>
> Right. There is little point in using it with SysV init though which,
> as you pointed out, has its own reboot/poweroff.
> 
> > Since you are using SysV init, doesn't it make more sense
> > to use _corresponding_ reboot from the same package?
> > I think SysV init provides those. Correct me if I'm wrong.
>
> I'm not, I'm using upstart, which in its present semiexperimental
> state does not provide means of rebooting. And I hear there are other
> "better" init replacements in the making.

Upstart source tree is 11540 kbytes. Do you use it because
you need your system to be small? This is not easy to believe.

> > Do you think it's beneficial for bbox to be able to mimic
> > SysVinit's reboot and it outweighs the additional of a CONFIG_xxx
> > and increased chances of users being confused?
>
> Well, my opinion here is that if you can make a busybox applet behave
> in as many [real life, might I add] situations as possible at low
> cost, then do it. Additional CONFIG_ options are cheap, all things
> considered. Chance of user getting confused is at absolute minimum: he
> won't even be offered to select these options unless he has HALT
> enabled and INIT disabled, in which case he might be interested in
> these options.

Okay. Let's apply it.

-               }
-               if (rc) {
-                       rc = kill(1, signals[which]);
+
+                       if (rc)
+                               rc = kill(1, signals[which]);

?! this change breaks it. Now we never send the signal
if !ENABLE_FEATURE_INITRD.

+               } else if (ENABLE_FEATURE_CALL_TELINIT) {
+                       /* runlevels:
+                        * 0 == shutdown
+                        * 6 == reboot */
+                       const char *const args[] = {

static?

+                               "telinit",

and if ENABLE_FEATURE_CALL_TELINIT is something completely different?

+                               which == 2 ? "6" : "0",
+                               NULL
+                       };
+
+                       rc = execvp(CONFIG_TELINIT_PATH, (char **)args);

Or just use execlp?


Please try attached patch.
-- 
vda
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 7.patch
Type: text/x-diff
Size: 2061 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20090727/b3a4cad3/attachment.bin>