[Toybox] [PATCH] timeout: --foreground, --preserve-status, and --signal.

[Rob Landley]

On 3/9/19 8:04 PM, enh via Toybox wrote:
> --signal is simply a synonym for the exiting -s.
> 
> --foreground disables functionality we didn't yet have: putting the
> child into a new process group. I've added the functionality and the
> flag to disable it.
> 
> --preserve-status also makes it clear that our exit statuses didn't match
> the coreutils version. In addition to callers that use --preserve-status
> to get away from this madness, I also have callers that check for
> specific exit values. This patch implements --preserve-status but also
> fixes all the other exit statuses.

Why are you open-coding xexec() here?

-  if (!(TT.pid = XVFORK())) xexec(toys.optargs+1);
-  else {
+  if (!FLAG(foreground)) setpgid(0, 0);
+
+  if (!(TT.pid = XVFORK())) {
+    char **argv = toys.optargs+1;
+
+    execvp(argv[0], argv);
+    perror_msg("failed to run '%s'", argv[0]);
+    toys.exitval = (errno == ENOENT) ? 127 : 126;
+    _xexit();
+  } else {
+    int status;
+

Presumably it's because something really really really needs the 126 error code?
(Because you need to distinguish between "I tried to run an arm binary on x86
without having set up the qemu binary association thing"?)

Oddly enough, installing the qemu package on ubuntu installs the file
associations (DO NOT WANT!) which means:

$ m68k/m68k-linux-musl-root/bin/toybox
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault (core dumped)
landley at halfbrick:~/mkroot/mkroot/output$ echo $?
139

The error return code is all over the place...

But if I try to execute a _directory_, I get 126. And presumably you have a
build script that cares? And want to open code it rather than teaching xexec()
to distinguish...

> (The "125" exit value is broken for toybox in the same way that
> `toybox grep --whoops ; echo $?` is. To fix this, we'd need some way to
> signal that command-line parsing failures should exit with a different
> value than the usual 1 --- 2 for grep, 125 for timeout. I've done as much
> as grep manages, and left a TODO.)

It's not a bug, it's a feature request. That said, toy_list->flags is an int,
we've used 10 of the 32 bits so far, and non-signal exit values are 7 bits.
(Signal exit is 127+signum, with "command not found" acting like signal 0)).

So we could probably just:

diff --git a/lib/args.c b/lib/args.c
index 54106c2..1ebc4f2 100644
--- a/lib/args.c
+++ b/lib/args.c
@@ -388,6 +388,8 @@ void get_optflags(void)
   // Option parsing is a two stage process: parse the option string into
   // a struct opts list, then use that list to process argv[];

+  toys.exitval = toys.which->flags >> 24;
+
   // Allocate memory for optargs
   saveflags = 0;
   while (toys.argv[saveflags++]);
@@ -495,6 +497,8 @@ notflag:
     help_exit("Needs %s-%s", s[1] ? "one of " : "", needs);
   }

+  toys.exitval = 0;
+
   if (CFG_TOYBOX_FREE) {
     llist_traverse(gof.opts, free);
     llist_traverse(gof.longopts, free);
diff --git a/lib/toyflags.h b/lib/toyflags.h
index bec8078..408c3ec 100644
--- a/lib/toyflags.h
+++ b/lib/toyflags.h
@@ -31,6 +31,9 @@
 // Suppress default --help processing
 #define TOYFLAG_NOHELP   (1<<9)

+// Error code to return if argument parsing fails (default 1)
+#define TOYFLAG_ARGFAIL(x) (x<<24)
+
 #if CFG_TOYBOX_PEDANTIC_ARGS
 #define NO_ARGS ">0"
 #else


(I should probably change the which->flags type to "unsigned" but didn't bother
in the above patch.)

Then you could just |TOYFLAG_ARGFAIL(125) in the command and it'd do the thing?
(During lib/args.c processing anyway.)

I still shake my head at _caring_ about this level of corner case? None of this
is specified anywhere that I'm aware of...

> Also add timeout tests. I couldn't think of an easy test for
> --foreground, so I tested that manually with strace.
>
> Also add some newlines to the `toybox --help` output to make it easier
> to find the different sections, and expand the section on durations to
> call out that fractions are supported as a matter of policy.
> 
> As long as timeout and sleep have text describing the duration syntax,
> make them the same. (Personally I'd remove both in favor of the `toybox
> --help` output, but as long as they're duplicated, keep them consistent.)
> 
> Also remove the SLEEP_FLOAT variant --- xparsetime means that sleep no
> longer requires floating point to support sub-second resolution.

I'd happily apply all that part, but you're rolling a bit much into one patch
here. :)

Rob