wrong output: find --help | grep BusyBox

[smalltalkman]

# find --help | grep BusyBox
BusyBox v1.37.0-FRP-5301-gda71f7c57 (2024-02-20 13:56:32 GMT)

Usage: find [-HL] [PATH]... [OPTIONS] [ACTIONS]

Search for files and perform actions on them.
First failed action stops processing of current file.
Defaults: PATH is current directory, action is '-print'

        -L,-follow      Follow symlinks
        -H              ...on command line only
        -xdev           Don't descend directories on other filesystems
        -maxdepth N     Descend at most N levels. -maxdepth 0 applies
                        actions to command line arguments only
        -mindepth N     Don't act on first N levels
        -depth          Act on directory *after* traversing it

Actions:
        ( ACTIONS )     Group actions for -o / -a
        ! ACT           Invert ACT's success/failure
        ACT1 [-a] ACT2  If ACT1 fails, stop, else do ACT2
        ACT1 -o ACT2    If ACT1 succeeds, stop, else do ACT2
                        Note: -a has higher priority than -o
        -name PATTERN   Match file name (w/o directory name) to PATTERN
        -iname PATTERN  Case insensitive -name
        -path PATTERN   Match path to PATTERN
        -ipath PATTERN  Case insensitive -path
        -regex PATTERN  Match path to regex PATTERN
        -type X         File type is X (one of: f,d,l,b,c,s,p)
        -executable     File is executable
        -perm MASK      At least one mask bit (+MASK), all bits (-MASK),
                        or exactly MASK bits are set in file's mode
        -mtime DAYS     mtime is greater than (+N), less than (-N),
                        or exactly N days in the past
        -atime DAYS     atime +N/-N/N days in the past
        -ctime DAYS     ctime +N/-N/N days in the past
        -mmin MINS      mtime is greater than (+N), less than (-N),
                        or exactly N minutes in the past
        -amin MINS      atime +N/-N/N minutes in the past
        -cmin MINS      ctime +N/-N/N minutes in the past
        -newer FILE     mtime is more recent than FILE's
        -inum N         File has inode number N
        -samefile FILE  File is same as FILE
        -size N[bck]    File size is N (c:bytes,k:kbytes,b:512 bytes(def.))
                        +/-N: file size is bigger/smaller than N
        -links N        Number of links is greater than (+N), less than (-N),
                        or exactly N
        -empty          Match empty file/directory
        -prune          If current file is directory, don't descend into it
If none of the following actions is specified, -print is assumed
        -print          Print file name
        -print0         Print file name, NUL terminated
        -exec CMD ARG ; Run CMD with all instances of {} replaced by
                        file name. Fails if CMD exits with nonzero
        -exec CMD ARG + Run CMD with {} replaced by list of file names
        -ok CMD ARG ;   Prompt and run CMD with {} replaced
        -delete         Delete current file/directory. Turns on -depth option
        -quit           Exit

[rmyorston]

The help message is being sent to stderr.

According to POSIX stderr should be used for diagnostic output when the exit code of a utility indicates an error.

That isn't the case here, as generating help text isn't an error.

However, sending help to stderr is what upstream BusyBox does, and for something this fundamental I wouldn't want busybox-w32 to diverge from upstream.

[avih]

sending help to stderr is what upstream BusyBox does

FWIW, I think this can be considered a bug.

There are also some other (non-busybox) utils which print help to stderr, but most utils do print the intentional help message to stdout (when using -h or --help).

When it's part of an error message (like a short usage line, etc), then it should, and typically does, print to stderr.

[avih]

@rmyorston If you're interested, this small enough patch seem to handle well the non-individual-applet case (the individual-applet case does seem to print to stdout, if I read it correctly - at individual.c, though I haven't tested it):

diff --git a/libbb/appletlib.c b/libbb/appletlib.c
index 7586f1ce1..81b25783d 100644
--- a/libbb/appletlib.c
+++ b/libbb/appletlib.c
@@ -143,6 +143,8 @@ static const char packed_usage[] ALIGN1 = { PACKED_USAGE };

 void FAST_FUNC bb_show_usage(void)
 {
+#define full_write2_str (xfunc_error_retval ? full_write2_str : full_write1_str)
+
    if (ENABLE_SHOW_USAGE) {
 #ifdef SINGLE_APPLET_STR
        /* Imagine that this applet is "true". Dont link in printf! */
@@ -193,6 +195,7 @@ void FAST_FUNC bb_show_usage(void)
 #endif
    }
    xfunc_die();
+
+#undef full_write2_str
 }

 #if ENABLE_PLATFORM_MINGW32 && NUM_APPLETS > 1 && \

If bloat is a concern due to replacing each full_write2_str call with more code (or the macro seemingly referencing itself), then it can be placed in a function instead, still with the same minimal diff at bb_show_usage.

EDIT (re bloat/macro-recursion): Or change the #define part to something like this, which saves 136 bytes in the x86 build compared to the above (and also seem to actually save 4 bytes compared to master!):

void FAST_FUNC bb_show_usage(void)
{
    ssize_t FAST_FUNC (*full_write_fn)(const char *) =
        xfunc_error_retval ? full_write2_str : full_write1_str;
#define full_write2_str full_write_fn

[smalltalkman]

The help message is being sent to stderr.

It's weird, but this worked for me: find --help 2>&1 | grep BusyBox

[avih]

@rmyorston do you want me to open a PR with a minimal patch similar to the suggestion above?

[rmyorston]

do you want me to open a PR

No thanks. This needs to go upstream and I'm considering how to do that.

[avih]

I'm considering how to do that.

Add this

    ssize_t FAST_FUNC (*full_write_fn)(const char *) =
        xfunc_error_retval ? full_write2_str : full_write1_str;

and change all the full_write2_str in this function to full_write_fn ?

[rmyorston]

Proposed upstream patches:

0001-libbb-use-full_write1_str-to-shrink-busybox_main.txt

0002-libbb-send-usage-messages-to-correct-stream.txt

[avih]

LGTM.

Nice sleuthing with the dup2 and full_write2_str at the first patch (I didn't try to verify that all messages go to the same stream as before). EDIT: I did go over it, I think it's fine.

At the second patch, personally I used the commit title "usage: print non-error help messages to stdout", which admittedly Is not too great, but it is a bit more explicit, though it doesn't matter much either way.

[avih]

At the 2nd patch:

-       )
+       ) {
+           xfunc_error_retval = 0;
            bb_show_usage();
+       }

Does this suggest that previously bb_show_usage() could exit with incorrect code?

If yes, then it should probably fixed in its own commit, and if no (I'd think it's "no", because it should default to 0), then I don't think this part is required?

[rmyorston]

Yes, the return code was wrong and the output went to stderr. xfunc_error_retval defaults to 1 (it's set in libbb/default_error_retval.c).

The change brings that call to bb_show_usage() into line with the other two in the same file. Since it ensures the usage message goes to the correct stream it seems appropriate to put it in the same commit.

[avih]

Yes, the return code was wrong

Hmm.. time --help does exit with code 0. I guess this change only applies when an applet is built individually (which I never tried)?

In this case, I think it's at least worth mentioning at the commit message that it also fixes incorrect exit code of individual applets with --help, but up to you.

[rmyorston]

I've just posted off the two patches unmodified. Let's see what happens.