FEATURE_PREFER_APPLETS should fail gracefully.

[Denys Vlasenko]

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 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 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.

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:

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?

> 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.



> 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?

-- 
vda



diff -ad -urpN busybox.4/include/libbb.h busybox.5/include/libbb.h
--- busybox.4/include/libbb.h   2011-01-31 05:50:35.000000000 +0100
+++ busybox.5/include/libbb.h   2011-02-02 02:51:26.000000000 +0100
@@ -867,11 +867,13 @@ int exists_execable(const char *filename
  * but it may exec busybox and call applet instead of searching PATH.
  */
 #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)
+int BB_EXECVP(const char *file, char *const argv[]) FAST_FUNC;
 #define BB_EXECLP(prog,cmd,...) \
-       execlp((find_applet_by_name(prog) >= 0) ? CONFIG_BUSYBOX_EXEC_PATH : prog, \
-               cmd, __VA_ARGS__)
+       do { \
+               if (find_applet_by_name(prog) >= 0) \
+                       execlp(bb_busybox_exec_path, cmd, __VA_ARGS__); \
+               execlp(prog, cmd, __VA_ARGS__); \
+       } while (0)
 #else
 #define BB_EXECVP(prog,cmd)     execvp(prog,cmd)
 #define BB_EXECLP(prog,cmd,...) execlp(prog,cmd, __VA_ARGS__)
diff -ad -urpN busybox.4/libbb/execable.c busybox.5/libbb/execable.c
--- busybox.4/libbb/execable.c  2011-01-31 05:50:35.000000000 +0100
+++ busybox.5/libbb/execable.c  2011-02-02 02:53:22.000000000 +0100
@@ -68,12 +68,12 @@ int FAST_FUNC exists_execable(const char
 }

 #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[])
+/* 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);
+       if (find_applet_by_name(file) >= 0)
+               execvp(bb_busybox_exec_path, argv);
+       return execvp(file, argv);
 }
 #endif