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

[enh]

On Sun, Mar 10, 2019 at 6:57 PM Rob Landley <rob at landley.net> wrote:
>
> 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"?)

it's the documented behavior, and (to my surprise) there are folks
actually checking the specific exit codes.

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

well, i was scared that this was only timeout. but you're right, other
GNU stuff seems to do the same.

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

lgtm, thanks.

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

not sure whether that's because you refuse to call GNU documentation
"specification" or because you refuse to run info(1) :-)

fwiw, busybox makes this 126/127 distinction too.

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

yeah, sorry. i did think about separating the sleep part out, but it
seemed like the motivation (keeping the two places that referred to
this in sync) was more obvious this way. amusingly, i forgot to remove
the TOYBOX_FLOAT dependency from timeout.c itself! i've sent a patch
for that just now... (and the other commands with TOYBOX_FLOAT look
legit.)

while does anyone really care about TOYBOX_FLOAT in 2019? and how much
binary size does floating point emulation cost them, as a percentage
of toybox?

> Rob