[Toybox] [PATCH] watch: flush the buffer each round.

[Rob Landley]

Hello from Minneapolis: https://mstdn.jp/@landley/112078501045637288

Still _really_ fried from my move, but at least chipping away at the email
backlog...

On 3/11/24 12:27, enh wrote:
> On Fri, Mar 8, 2024 at 5:43 PM Rob Landley <rob at landley.net> wrote:
>>
>> I remember when the xprintf() family would do a flush and check for errors each
>> write. That's why code like:
>>
>>         dprintf(1, "%c", pad<cmdlen ? '*' : ' ');
>>         if (width) xputs(ss+(width>ctimelen ? 0 : width-1));
>>         if (yy>=3) dprintf(1, "\r\n");
>>
>> Was allowable.
>>
>> I also remember when we had an xflush() that would catch errors if stdout
>> barfed, instead of calling fflush() and ignoring the return code.
> 
> yeah, i didn't understand why you removed xflush().

The commit comment on 3e0e8c687eee tried to explain: years ago xflush() got
broken to have a "don't actually flush" argument, and thus didn't do what its
name said anymore, so I renamed it to what it did (check errors and nothing
more) and added a manual flush in the places that was passing in the "actual
flush" argument. (So that change shouldn't have made it worse.)

My original approach to FILE * was to just always flush (ala xprintf) but there
were objections to that, which I said would open a can of micromanagement worms.
Then I changed the default buffer type to be unbuffered instead, and there were
objections to that, which I said would open a can of micromanagement worms...

> but none of this
> is relevant here --- the problem here is that we're not flushing _at
> all_, so ToT watch is just broken.

Micromanagement, yes.

>> This code is mixing dprintf(1, ...) with printf() and fflush(stdout) in a way
>> that seems unlikely to end well, and I would like to think it through when I'm
>> not moving house. (Or have somebody whose brain isn't jello reassure me that
>> it's coherent.)
> 
> not in the ToT copy it isn't. maybe you have local changes?

Hmmm, yup looks like I was already fixing this command up to not use FILE * at
all anymore (what is this, my fourth attempt to get away from FILE * buffering
damage in a systematic way?) and got distracted partway through...

> /tmp/toybox$ grep dprintf toys/other/watch.c
> /tmp/toybox$
> 
>> Ideally, I would wean it entirely off of the conceptually broken FILE * output
>> nonsense entirely to dprintf() and write(), because it's intentionally doing
>> progressive output and trying to micromanage cache management there is unending
>> pain. (We need a stdout with the nagle algorithm. Why did they only bother to do
>> that for network sockets?)
> 
> aye, you keep saying, but that doesn't exist yet, and watch is broken today :-)

The date on the file is February 10th, so I was working on this last month.

Alas, my life's been a bit hectic recently. Trying to shovel out now...

> that said, my previous patch was suboptimal --- there are `continue`s
> in the loop, so the flush makes more sense at the top. also i noticed
> by inspection trying to work out what you meant about mixing stdio and
> direct fd access (i don't use watch myself; this was a reported bug)
> that -b was broken.

My brain is currently trying to filk the Steven Universe song "other friends" to
be a demand about test cases and reproduction sequences, but reading the code I
see the typo.

> so the new patch (attached) is a better fix. but
> that fixes the normal case and -b for me...

Spraying down the code with manual fflush() calls strikes me as a code smell
that means "this will break the next time anybody touches anything".

But I mentioned still being fried from the move, which makes me extra irritable.
(When did U-haul become a scam? That's new since the last time I used them...)

Rob