On 02/01/2011 08:24 PM, Denys Vlasenko wrote: > On Tuesday 01 February 2011 19:00, Rob Landley wrote: >> This thing has bloated out of control and I no longer even pretend to >> understand it. I just thought I'd pass on the feature suggestion. > > One reason where bloat is a result of ill-fated attempt > to make NOFORK applets to properly background on Ctrl-Z.
The shell "read" command doesn't properly background with Ctrl-Z. Not even in bash. (The only possibly sane way I can think of making that work is to fork from the signal handler. It stops being a builtin when you need to suspend it. And even this really depends on your definition of "sane", since forking from an exception handler is unlikely to be anything like portable, assuming even Linux supports it.) However, NOFORK really isn't worth a whole lot of effort. It's a minor optimization that doesn't actualy buy you much. Fork simply is not an expensive operation on most Linux systems. When I brought it up in the first place, I was thinking of the pile of commands that ALREADY have to be nofork, like "export" and "cd" and "exit". 90% of my motivation was "is there any way I can _avoid_ having a second applet list in the shell and just teach the main applet list that some of these are just function calls rather than a fork? My motivation for nofork wasn't to speed things up (the _vast_ majority of of the fork/exec time is going to be exec), it was to reduce the complexity of busybox by collapsing together two redundant applet lists. I.E. my primary motivation was always to simplify. How can I do more with less? Yeah, commands like "echo" are tempting to add to a nofork category once you've got it, "this is basically a function, why can't I just call it?" Except in the case where somebody pipes the output to something that blocks, then it's not so simple. Anything that produces _output_ turns out to be a can of worms. (That's why.) In general, there's not a whole lot of virtue in making more applets nofork. The virtue was in eliminating the separate applet dispatch table built into your shell implementation. I vaguely recall we had a thread about this on the list, and google pulled up a first hit of: http://lists.busybox.net/pipermail/busybox/2009-January/068158.html Now applets you don't have to _exec_ are pretty keen. I realize that on nommu systems you pretty much have to follow fork with an exec or an exit, and thus "nofork" and "noexec" get conflated if you try to support MMU. But keep in mind this is merely an _optimization_, and "sucks to be a nommu" is a long book with many chapters that this would be a very small appendix to. > The good example is rm. It's a good candidate for NOFORK... > ...until you remember that it has -i "ask before delete" option! > What if user Ctrl-Z'es it?? Normally, rm process backgrounds. > But if NOFORK execution is enabled, then there is NO rm process! I note that in toybox, the common infrastructure parsed the command line options before calling the relevant command_main, so it would have been easy for toysh to have an option mask for each applet that would only be nofork if none of those bits were set. (Yes, I did think about that sort of thing, way back when. Never got around to implementing most of it...) Still not all that worth it, though. I was going for simplifying and collapsing together infrastructure so there was a single codepath that was just really generic and flexible (without a zillion special cases). Using nofork as a reason to _add_ complexity is sort of missing the point. > I had it sort-of-working with much hacking, but after some > more musing on it I desided that the hack to make it work > is far too ugly and also fragile. I eventually removed it > in commits c4ada7934307c6ef0b5b207df13a54fecd4ad81d > and d5762932fbcbc0a385047945276f10e2f3fea12d. > 1.13.x are the last versions which had that hack. It is gone > in 1.14.x. Good. > But the support code remains for now (struct nofork_save_area > and functions which save/restore it). > > What do you think about "rm -i"? Should we simply not support > Ctrl-Z'ing it? Currently, with this .config: I think "noexec" is low hanging fruit, but "nofork" is a can of worms. > CONFIG_FEATURE_PREFER_APPLETS=y > CONFIG_FEATURE_SH_STANDALONE=y > CONFIG_FEATURE_SH_NOFORK=y > > in hush, pressing ^Z or ^C at "rm -i FILE" prompt does nothing > (merely shows ^Z and ^C). Is this acceptable? If not, > what do you propose? Making rm a NOEXEC applet rather than a NOFORK applet. NOFORK means "shell builtin". NOEXEC means "I know for a fact we don't need to reinitialize global state the previous applet may have stomped on". Note that we don't even have to be careful about freeing memory and closing files, since the OS does that. I'd need to look more closely at the _exit() vs exit() thing, but if the child and parent aren't doing a CLONE_FILES thing then as long as stdio got flushed before the fork it's probably ok...? >> I did dig a bit, by the way. In libbb/execable.c (what kind of name is >> that?) >> >> #if ENABLE_FEATURE_PREFER_APPLETS >> /* just like the real execvp, but try to launch an applet named 'file' first >> */ >> int FAST_FUNC bb_execvp(const char *file, char *const argv[]) >> { >> return execvp(find_applet_by_name(file) >= 0 ? >> bb_busybox_exec_path : file, >> argv); >> } >> #endif >> >> Define "first"? That attempts to exec exactly one thing, with no fallback. > > Bug. Fixing. Probably the bug I was reporting. :) > > >> And while we're at it: >> >> #if ENABLE_FEATURE_PREFER_APPLETS >> int bb_execvp(const char *file, char *const argv[]) FAST_FUNC; >> #define BB_EXECVP(prog,cmd) bb_execvp(prog,cmd) >> #define BB_EXECLP(prog,cmd,...) \ >> execlp((find_applet_by_name(prog) >= 0) ? >> CONFIG_BUSYBOX_EXEC_PATH : prog, \ >> cmd, __VA_ARGS__) >> #else >> #define BB_EXECVP(prog,cmd) execvp(prog,cmd) >> #define BB_EXECLP(prog,cmd,...) execlp(prog,cmd, __VA_ARGS__) >> #endif >> >> Thats an abomination. The only user of bb_execvp() is that macro. So >> you create an unnecessary wrapper (inside an #ifdef) which you then >> rename with a macro (inside an #ifdef, just so the #ifdef occurs twice). >> In one of them, you have a function wrapper. Write next to it, ou're >> doing the wrapping in the macro. There is NO REASON FOR ANY OF THIS. > > How does this look? > Not really an improvement. (And all caps function name is scar tissue.) I remember when we had something like bb_exec(), and channeled everything through it. Having two bb_exec variants where one isn't a convenience wrapper around the other would make me step back and go "why?" even without the #ifdef. However, I haven't really got enough context to step back and go "why" anymore. Rob _______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
