[Toybox] [PATCH] xputs: Do flush

[Rob Landley]

On 5/18/24 21:53, Yi-Yo Chiang wrote:
> What I wanted to address with this patch are:
> 
> 1. Fix this line of
> xputs() https://github.com/landley/toybox/blob/master/toys/net/microcom.c#L113
> The prompt text is not flushed immediately, so it is not shown to the user until
> the escape char is entered (which defeats the purpose of the prompt, that is to

I agree you've identified two problems (unflushed prompt, comment not matching
code) that both need to be fixed. I'm just unhappy with the solutions, and am
concerned about a larger design problem.

I implemented TOYFLAG_NOBUF and applied it to this command. The result compiles
but I'm not near serial hardware at the moment, does it fix the problem for you?

Trying to fix it via micromanagement (adding more flushing and switching some
but not all output contexts in the same command between FILE * and file
descriptor) makes my head hurt...

Adding flushing to xputs() is an Elliott question is because he's the one who
can presumably keep stdio buffer semantics in his head. They make zero sense to
me. I added a flush to xputsn() because in line buffering mode it was the
"dangerous" one that had no newline thus no flush, but then when we go to block
buffering mode xputs() needs a flush just like xputsn() would, and MAYBE it's
good to have the flush because in line buffer mode it would be a NOP? Except the
command selected block buffering mode precisely BECAUSE it didn't want to flush
each line, so why should xputs() do it when the command asked not to? And if
xputs() is doing it, why is xprintf() _not_ doing it? And if xprintf() _is_
doing it, then we're back to basically not buffering the output...

> tell the user what the escape char is) and stdout is flushed by handle_esc.
> To fix this we either make xputs() flush automatically, or we just add a single
> line of manual flush() after xputs() in microcom.c.
> Either is fine with me.

When I searched for the first xputs in microcom I got:

  xputsn("\r\n[b]reak, [p]aste file, [q]uit: ");
  if (read(0, &input, 1)<1 || input == CTRL('D') || input == 'q') {

Which is a separate function (the n version is no newline, it does not add the
newline the way libc puts() traditionally does), with its own flushing
semantics: xputsn() doesn't call xputs(), and neither calls or is called by
xprintf(). "Some functions flush, some functions don't" is a bit of a design
sharp edge.

The larger problem is manual flushing of the output buffer is a terrible
interface, and leads to missing error checking without which a command won't
reliably exit when its output terminal closes because the whole SIGPIPE thing
was its own can of worms that even bionic used to manually mess with. Which is
why I originally made toybox not ever do that (systemic fix) but I got
complaints about performance.

Your other patch changes a bunch of xprintf() to dprintf() which is even _more_
fun because dprintf() writes directly to the file descriptor (bypassing the
buffer in the libc global FILE * instance "stdio"), which means in the absence
of manual flushing the dprintf() data will go out first and then the stdio data
will go out sometime later, in the wrong order. Mixing the two output formats
tends to invert the order that way, unless you disable output buffering.

I like systematic solutions that mean the problem never comes up again. Elliott
has been advocating the whack-a-mole "fix them as we find them" approach here
which I am not comfortable with. I've been leaning towards adding a
TOYFLAG_NOBUF so we can select a third buffering type, and go back to "do not
buffer output at all, flush after every ANSI write function" for at least some
commands I'd like to be able to reason about. Especially for commands like this
where output performance really doesn't seem like it should be an issue.

And OTHER implementations can't consistently get this right, which is why whether:

  for i in {1..100}; do echo -n .; sleep .1; done | less

Produces any output before 10 seconds have elapsed is potluck, and varies from
release to release of the same distro.

Oh, and the gnu/crazies just came up with a fourth category of write as a
gnu/extension: flush after NUL byte.

https://lists.gnu.org/archive/html/coreutils/2024-03/msg00016.html

It's very gnu to fix "this is too complicated to be reliable" by adding MORE
complication. Note how the problem WE hit here was 1) we didn't ask for LINEBUF
mode, 2) \r doesn't count as a line for buffer flushing purposes anyway, 3) the
new feature making it trigger on NUL instead _still_ wouldn't make \r count as a
line for buffer flushing purposes.

My suggestion for a "proper fix" to the problem _category_ of small writes being
too expensive was to have either libc or the kernel use nagle's algorithm for
writes from userspace, like it does for network connections. (There was a fix to
this category of issue decades ago, it just never got applied HERE.)

But that hasn't been popular, and it's a pain to implement in userspace (because
we don't have access to mulitple cheap timers like the kernel does, we need to
take a signal and there's both a limited number of signals). So that's not what
the ANSI committee decided to do in 1988, instead overlaying terrible semantics
on Ken/Dennis/Doug's elegant original design.

Now that the kernel has vdso, this seems to me like an obvious use for it: have
a vdso write() function detect fd 1 and write the bytes(s) into a page size ring
buffer with incremented output position indicator, maybe with the buffer taking
a soft fault on first write to enable the kernel timer to flush it 1/10 second
later (whatever the existing nagle timeout is), and big writes after small
writes could even become an iovec or something (maybe using cmpxchg to rewind
the buffer position indicator if the timer expiration hasn't advanced it yet) to
avoid extra syscalls when flushing the to buffer full instead of timer expiration.

Then userspace does the obvious thing and the kernel makes it fast. As
Ken/Dennis/Doug intended.

> 2. The comment string before xputs() is misleading and I wanted to fix that.
> I was misled by the comment line, and incorrectly thought xputs() was flushing.

Yeah, my bad. It's changed multiple times because of arguments about
performance. It's xputsn() and xputsl() that are currently flushing. (Which made
sense when "line buffer vs no buffer" were the options, vs block buffer vs line
buffer and you couldn't ask for NOT buffered. Commit 3e0e8c687eee back in January.)

Oh, and I've had similar arguments on the READ FILE * side, wanting getline() to
pass along the leftover readahead data after \n to child processes, ala "read i;
zcat > $i" which has the problem of getline() reading a block of data into the
FILE * buffer which then isn't available to zcat.

I eventually (https://landley.net/notes-2023.html#24-06-2023) worked out that on
pipes you can use O_DIRECT to prevent collating and thus most instances of
readahead, and on seekable files you can fseeko() on the file pointer to force
it to back up to where it THINKS it is and thus make the readahead data
available through the filehandle again, and that SHOULD cover the majority of
cases the shell cares about?

But that was AFTER I complained at the posix guys that there's no portable way
to ask an input FILE * how much data is in its buffer and wound up with SIX
different contexts (two standards committees, three linux library maintainers,
plus BSD) pointing fingers at each other, which I ranted about in my blog:

https://landley.net/notes-2022.html#19-04-2022

Thus the old saying that there are two hard problems in computer science:
naming things, cache invalidation, and off by one errors.

> I only discovered otherwise after iterations of trial-and-error.
> To fix this is to either make xputs() flush automatically, or just update the
> comment string to clarify that xputs() does _not_ flush.
> Again either path is fine with me. I just merged the fixes of the two problems
> into one because they share the former solution. Plus xputsl() and xputsn() are
> also flushing, so I thought / believed xputs() was mistakenly made non-flushing.

Even xprintf() _used_ to flush. All the xout() functions did. And then grep was
too slow... (See "historical performance complaints", above.)

> Since the intention _is_ to keep xputs() non-flushing, how about we just update
> the comment of xputs() (2), and add the line of flush in microcom.c (1)?

As I said, I did TOYFLAG_NOBUF and hope it works for you?

I don't know if xputs() should have the flushing added or the comment removed,
that's a design question for Elliott.

Rob