[Toybox] Fix build error after 'make allyesconfig'

[Rob Landley]

On 01/12/2016 03:26 AM, Lipi C. H. Lee wrote:
> Rob,
> 
> If try to 'make defconfig', CFG_TOYBOX_FORK is 1 in 'generated/config.h'.
> and CFG_TOYBOX_FORK is 0 in generated/config.h, after 'make allyesconfig'.

Yes, becaue allyesconfig forces:

config TOYBOX_MUSL_NOMMU_IS_BROKEN
        bool "Workaround for musl-libc breakage on nommu systems."
        default n

Which switches off TOYBOX_FORK, thus forcing the nommu paths to be
taken. (Musl provides a broken fork() that returns failure every time,
to prevent you from detecting this sort of thing at compile time. Musl's
maintainer insists that nommu workarounds must always be compiled into
the program on systems with mmu, for reasons that go whoosh over my head.)

> If CFG_TOYBOX_FORK is 0, xfork() function code is not included in object
> file  because of #if CFG_TOYBOX_FORK in 'lib/portability.c'.
> So link error happens after 'make allyesconfig'.

Yes. Many of the "pending" commands don't support nommu yet.

I just pushed a commit updating the "make help" text to try to clarify
what defconfig, allyesconfig, and allnoconfig are for.

> If you want xfork() code to be added in its object file without caring
> about CFG_TOYBOX_FORK's value,

Which would defeat the purpose of CFG_TOYBOX_FORK.

> you would change #if CFG_TOYBOX_FORK to #ifdef CFG_TOYBOX_FORK and only
> this patch will work for 'make allyesconfig' without first patch.

CFG_TOYBOX_FORK is always #defined, to either 0 or 1. #ifdef on a symbol
that is always defined is always true, you're effectively removing the
test. #if is the correct test on a CFG_ symbol (which is not a CONFIG_
symbol, which has different semantics, either #defined or not and thus
not testable from normal C if (CFG_BLAH) stuff();)

> But, my first patch is referred to below lines in 'lib/xwrap.c'.
> 
> 180   // Child process.
> 
> 181   if (!(pid = CFG_TOYBOX_FORK ? xfork() : XVFORK())) {
> 
> 182     // Dance of the stdin/stdout redirection.

In the XVFORK() case, the code won't work, because vfork() freezes the
parent until the child either calls execve() or _exit().

When I first added nommu support to toybox, I taught a library function
to re-exec the current process via the /proc/self/exe symlink (because
my suggestion years ago that execve() with a NULL filename re-exec the
current process was never implemented in the kernel). It's
xpopen_both(), when called with a NULL arvg[] argument. (The second
argument is pipes to/from the child's stdin, if you pass 0 for that then
the child inherits the parent stdin/out.)

Currently the only user of xpopen(0, X) is cpio -p mode, but I
implemented and tested it there so I'd have the infrastructure an make
sure it worked for cases like this that need it.

That said, exec("/proc/self/exe", argv[]) re-starts the command from the
beginning, and the signal that this is a child invocation is that
toys.stacktop (our "recursion is eating too much stack, time for a fresh
exec" metric) is 0. Thus cpio has:

  // Passthrough, parent stays in original dir and generates archive
  // to pipe, child chdir to new dir, reads archive from stdin (pipe).
  if (TT.pass) {
    if (toys.stacktop) {
      // xpopen() doesn't return from child due to vfork(), it restarts
      // with !toys.stacktop
      pid = xpopen(0, &pipe, 0);
      afd = pipe;
    } else {
      // child
      toys.optflags |= FLAG_i;
      xchdir(TT.pass);
    }
  }

I.E. if (toys.stacktop) we're the parent, else we're the child. (This
uses the pipe argument because passing data from parent to child is the
point there. xpopen() is just a wrapper around xopen_both() that sets up
only one of the two pipe[] arguments and signals that the other should
inherit stdin/stdout, I wrapped it so users don't have to memorize the
signaling, they can just xpopen(0, &int, stdin-or-stdout);

So what I need to do to make this work with nommu is A) replace xfork()
with xopen_both(0,0), B) have an if (!toys.stacktop) stanza near the
start of command_main() to intercept child execution and do the child
bits there.

In the meantime, the command builds if you haven't selected nommu. The
build break tells me that nommu isn't implemented there yet. Your
proposed changes do NOT implement nommu support, they make the build
break go away but produce broken code.

What you can instead do is add "depends on TOYBOX_FORK" to the kconfig
stuff near the top, the way nbd_client.c or netcat.c have in some of
their options. That makes this command drop out entirely from
"allyesconfig", so it won't get built.

(If that seems like a strange way for allyesconfig to behave, welcome to
kconfig dependency resolution. Allyesconfig was only _ever_ a starting
point you then adjust to meet your needs with menuconfig.)

Rob