<div dir="ltr"><div>Thanks! Adding TOYFLAG_NOBUF worked.</div><div><br></div><div>I feel the same way about "manual flushing of the output buffer is a terrible interface". I asked myself the question "Why am I manually flushing so much? There must be a better way..." multiple times when I wrote the other patch that does s/xprintf/dprintf/, s/xputs/xputsn/</div><div><br></div><div><br></div><div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 20, 2024 at 8:36 PM enh <<a href="mailto:enh@google.com" target="_blank">enh@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Sun, May 19, 2024 at 10:56 PM Rob Landley <<a href="mailto:rob@landley.net" target="_blank">rob@landley.net</a>> wrote:<br>
><br>
> On 5/18/24 21:53, Yi-Yo Chiang wrote:<br>
> > What I wanted to address with this patch are:<br>
> ><br>
> > 1. Fix this line of<br>
> > xputs() <a href="https://github.com/landley/toybox/blob/master/toys/net/microcom.c#L113" rel="noreferrer" target="_blank">https://github.com/landley/toybox/blob/master/toys/net/microcom.c#L113</a><br>
> > The prompt text is not flushed immediately, so it is not shown to the user until<br>
> > the escape char is entered (which defeats the purpose of the prompt, that is to<br>
><br>
> I agree you've identified two problems (unflushed prompt, comment not matching<br>
> code) that both need to be fixed. I'm just unhappy with the solutions, and am<br>
> concerned about a larger design problem.<br>
><br>
> I implemented TOYFLAG_NOBUF and applied it to this command. The result compiles<br>
> but I'm not near serial hardware at the moment, does it fix the problem for you?<br>
><br>
> Trying to fix it via micromanagement (adding more flushing and switching some<br>
> but not all output contexts in the same command between FILE * and file<br>
> descriptor) makes my head hurt...<br>
><br>
> Adding flushing to xputs() is an Elliott question is because he's the one who<br>
> can presumably keep stdio buffer semantics in his head. They make zero sense to<br>
> me. I added a flush to xputsn() because in line buffering mode it was the<br>
> "dangerous" one that had no newline thus no flush, but then when we go to block<br>
> buffering mode xputs() needs a flush just like xputsn() would, and MAYBE it's<br>
> good to have the flush because in line buffer mode it would be a NOP? Except the<br>
> command selected block buffering mode precisely BECAUSE it didn't want to flush<br>
> each line, so why should xputs() do it when the command asked not to? And if<br>
> xputs() is doing it, why is xprintf() _not_ doing it? And if xprintf() _is_<br>
> doing it, then we're back to basically not buffering the output...<br>
<br>
this to me was exactly why it should be "everything flushes" or<br>
"nothing flushes". not "some subset of calls for some subset of<br>
inputs", because no-one can keep all the special cases in their heads.<br>
and "everything flushes" is problematic from a performance<br>
perspective, so "nothing flushes" wins by default. (but, yes, when we<br>
have our own kernel, have a time-based component to buffering layer's<br>
flushing is an interesting idea :-) )<br>
<br>
> > tell the user what the escape char is) and stdout is flushed by handle_esc.<br>
> > To fix this we either make xputs() flush automatically, or we just add a single<br>
> > line of manual flush() after xputs() in microcom.c.<br>
> > Either is fine with me.<br>
><br>
> When I searched for the first xputs in microcom I got:<br>
><br>
>   xputsn("\r\n[b]reak, [p]aste file, [q]uit: ");<br>
>   if (read(0, &input, 1)<1 || input == CTRL('D') || input == 'q') {<br>
><br>
> Which is a separate function (the n version is no newline, it does not add the<br>
> newline the way libc puts() traditionally does), with its own flushing<br>
> semantics: xputsn() doesn't call xputs(), and neither calls or is called by<br>
> xprintf(). "Some functions flush, some functions don't" is a bit of a design<br>
> sharp edge.<br>
<br>
(yeah, exactly.)<br>
<br>
> The larger problem is manual flushing of the output buffer is a terrible<br>
> interface, and leads to missing error checking without which a command won't<br>
> reliably exit when its output terminal closes because the whole SIGPIPE thing<br>
> was its own can of worms that even bionic used to manually mess with. Which is<br>
> why I originally made toybox not ever do that (systemic fix) but I got<br>
> complaints about performance.<br>
><br>
> Your other patch changes a bunch of xprintf() to dprintf() which is even _more_<br>
> fun because dprintf() writes directly to the file descriptor (bypassing the<br>
> buffer in the libc global FILE * instance "stdio"), which means in the absence<br>
> of manual flushing the dprintf() data will go out first and then the stdio data<br>
> will go out sometime later, in the wrong order. Mixing the two output formats<br>
> tends to invert the order that way, unless you disable output buffering.<br></blockquote><div><br></div><div><br></div><div>Which is the reason I replaced those all with the "flush" functions (xputsn) or direct fd-write functions (dprintf), so that their order won't shuffle.</div><div>Anyway the problem is moot now that we have TOYFLAG_NOBUF.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> I like systematic solutions that mean the problem never comes up again. Elliott<br>
> has been advocating the whack-a-mole "fix them as we find them" approach here<br>
> which I am not comfortable with. I've been leaning towards adding a<br>
> TOYFLAG_NOBUF so we can select a third buffering type, and go back to "do not<br>
> buffer output at all, flush after every ANSI write function" for at least some<br>
> commands I'd like to be able to reason about. Especially for commands like this<br>
> where output performance really doesn't seem like it should be an issue.<br>
<br>
+1 --- an inherently interactive program like this seems reasonable to<br>
be unbuffered. (except for file transfer?)<br>
<br>
> And OTHER implementations can't consistently get this right, which is why whether:<br>
><br>
>   for i in {1..100}; do echo -n .; sleep .1; done | less<br>
><br>
> Produces any output before 10 seconds have elapsed is potluck, and varies from<br>
> release to release of the same distro.<br>
><br>
> Oh, and the gnu/crazies just came up with a fourth category of write as a<br>
> gnu/extension: flush after NUL byte.<br>
><br>
> <a href="https://lists.gnu.org/archive/html/coreutils/2024-03/msg00016.html" rel="noreferrer" target="_blank">https://lists.gnu.org/archive/html/coreutils/2024-03/msg00016.html</a><br>
<br>
(fwiw, i think that was just some internet rando asking for it, no?<br>
and they didn't actually implement it?)<br>
<br>
> It's very gnu to fix "this is too complicated to be reliable" by adding MORE<br>
> complication. Note how the problem WE hit here was 1) we didn't ask for LINEBUF<br>
> mode, 2) \r doesn't count as a line for buffer flushing purposes anyway, 3) the<br>
> new feature making it trigger on NUL instead _still_ wouldn't make \r count as a<br>
> line for buffer flushing purposes.<br>
><br>
> My suggestion for a "proper fix" to the problem _category_ of small writes being<br>
> too expensive was to have either libc or the kernel use nagle's algorithm for<br>
> writes from userspace, like it does for network connections. (There was a fix to<br>
> this category of issue decades ago, it just never got applied HERE.)<br>
><br>
> But that hasn't been popular, and it's a pain to implement in userspace (because<br>
> we don't have access to mulitple cheap timers like the kernel does, we need to<br>
> take a signal and there's both a limited number of signals).<br>
<br>
do you run on anything that doesn't have real-time signals? i was<br>
going to say that -- since toybox is a closed world -- you could just<br>
use SIGUSR2, but i see that dhcp is already using that! but if you can<br>
assume real-time signals, there are plenty of them...<br>
<br>
> So that's not what<br>
> the ANSI committee decided to do in 1988, instead overlaying terrible semantics<br>
> on Ken/Dennis/Doug's elegant original design.<br>
><br>
> Now that the kernel has vdso, this seems to me like an obvious use for it: have<br>
> a vdso write() function detect fd 1 and write the bytes(s) into a page size ring<br>
> buffer with incremented output position indicator, maybe with the buffer taking<br>
> a soft fault on first write to enable the kernel timer to flush it 1/10 second<br>
> later (whatever the existing nagle timeout is), and big writes after small<br>
> writes could even become an iovec or something (maybe using cmpxchg to rewind<br>
> the buffer position indicator if the timer expiration hasn't advanced it yet) to<br>
> avoid extra syscalls when flushing the to buffer full instead of timer expiration.<br>
><br>
> Then userspace does the obvious thing and the kernel makes it fast. As<br>
> Ken/Dennis/Doug intended.<br>
><br>
> > 2. The comment string before xputs() is misleading and I wanted to fix that.<br>
> > I was misled by the comment line, and incorrectly thought xputs() was flushing.<br>
><br>
> Yeah, my bad. It's changed multiple times because of arguments about<br>
> performance. It's xputsn() and xputsl() that are currently flushing. (Which made<br>
> sense when "line buffer vs no buffer" were the options, vs block buffer vs line<br>
> buffer and you couldn't ask for NOT buffered. Commit 3e0e8c687eee back in January.)<br>
><br>
> Oh, and I've had similar arguments on the READ FILE * side, wanting getline() to<br>
> pass along the leftover readahead data after \n to child processes, ala "read i;<br>
> zcat > $i" which has the problem of getline() reading a block of data into the<br>
> FILE * buffer which then isn't available to zcat.<br>
><br>
> I eventually (<a href="https://landley.net/notes-2023.html#24-06-2023" rel="noreferrer" target="_blank">https://landley.net/notes-2023.html#24-06-2023</a>) worked out that on<br>
> pipes you can use O_DIRECT to prevent collating and thus most instances of<br>
> readahead, and on seekable files you can fseeko() on the file pointer to force<br>
> it to back up to where it THINKS it is and thus make the readahead data<br>
> available through the filehandle again, and that SHOULD cover the majority of<br>
> cases the shell cares about?<br>
><br>
> But that was AFTER I complained at the posix guys that there's no portable way<br>
> to ask an input FILE * how much data is in its buffer and wound up with SIX<br>
> different contexts (two standards committees, three linux library maintainers,<br>
> plus BSD) pointing fingers at each other, which I ranted about in my blog:<br>
><br>
> <a href="https://landley.net/notes-2022.html#19-04-2022" rel="noreferrer" target="_blank">https://landley.net/notes-2022.html#19-04-2022</a><br>
><br>
> Thus the old saying that there are two hard problems in computer science:<br>
> naming things, cache invalidation, and off by one errors.<br>
><br>
> > I only discovered otherwise after iterations of trial-and-error.<br>
> > To fix this is to either make xputs() flush automatically, or just update the<br>
> > comment string to clarify that xputs() does _not_ flush.<br>
> > Again either path is fine with me. I just merged the fixes of the two problems<br>
> > into one because they share the former solution. Plus xputsl() and xputsn() are<br>
> > also flushing, so I thought / believed xputs() was mistakenly made non-flushing.<br>
><br>
> Even xprintf() _used_ to flush. All the xout() functions did. And then grep was<br>
> too slow... (See "historical performance complaints", above.)<br>
><br>
> > Since the intention _is_ to keep xputs() non-flushing, how about we just update<br>
> > the comment of xputs() (2), and add the line of flush in microcom.c (1)?<br>
><br>
> As I said, I did TOYFLAG_NOBUF and hope it works for you?<br>
><br>
> I don't know if xputs() should have the flushing added or the comment removed,<br>
> that's a design question for Elliott.<br>
<br>
comment removed makes sense to me?<br>
<br>
i like the x<foo>() => "it's <foo>, but exits on error" pattern. super<br>
consistent and easy to reason about, and easy to read when you know<br>
it, and easy to fix code that should/shouldn't error in either<br>
direction, by adding or removing the 'x'. (one of the biggest problems<br>
with the old xflush() was that it mostly _didn't_ flush! xferror() is<br>
a weird name, but at least it does what it says on the tin :-) )<br>
<br>
> Rob<br>
> _______________________________________________<br>
> Toybox mailing list<br>
> <a href="mailto:Toybox@lists.landley.net" target="_blank">Toybox@lists.landley.net</a><br>
> <a href="http://lists.landley.net/listinfo.cgi/toybox-landley.net" rel="noreferrer" target="_blank">http://lists.landley.net/listinfo.cgi/toybox-landley.net</a><br>
</blockquote></div></div></div>