Hi Collin,
> 'date' doesn't diagnose write errors immediately. That
> behavior should be changed, in my opinion.
>
> If you allowed it to run for however long it takes (hours?) to generate
> output, you would see a write error:
>
> $ strace date +`python -c 'print("%2147483648c" * 10000)'` 2>&1 \
> > /dev/full | head -n 100
> [...]
> write(1, " "..., 4096) = -1 ENOSPC (No
> space left on device)
> write(1, " "..., 4096) = -1 ENOSPC (No
> space left on device)
> write(1, " "..., 4096) = -1 ENOSPC (No
> space left on device)
> write(1, " "..., 4096) = -1 ENOSPC (No
> space left on device)
> write(1, " "..., 4096) = -1 ENOSPC (No
> space left on device)
>
> This patch checks for an error on the stream and returns early if there
> is one:
>
> $ time ./src/date +`python -c 'print("%2147483648c" * 10000)'` > /dev/full
> date: write error: No space left on device
>
> real 0m0.019s
> user 0m0.012s
> sys 0m0.007s
Two considerations:
1) The proposed patch changes the return value of fprintftime(). Where
previously the return value (number of bytes output to the stream)
was independent of exterior conditions, now it depends on such conditions.
Thus, coding patterns like the following are not valid any more:
1. Calling fprintftime with some arguments, writing to a log file.
2. Allocate RETVALUE + 1 bytes of memory.
3. Calling fprintftime with the same arguments, writing to an in-memory
stream with that bounded memory instead.
I'm not saying that this is a likely coding pattern. But it's a possible
one, and that sends a red flag.
So, something is wrong with the return value.
The following are possible mitigations:
a. Change the specification in fprintftime.h to state that the return
value is unreliable.
b. Change the code so that after ferror(fp) was encountered, it continues
the loop, just without doing output any more.
c. Change the return value to 'void'.
a. is not a significant mitigation, because what will the caller do? Most
likely nothing.
b. This is effectively equivalent (only slightly optimized) to what we
have now.
c. This will cause compilation errors in two existing packages [1][2].
Which is undesired for Gnulib modules.
In summary, this patch causes semantic trouble that is hard to fix.
2) What kind of patch is this? A bug fix or an optimization?
As I see it, it is merely an optimization, and in particular one for a
contrived, unrealistic use-case. 99.999999% of the 'date' command use
a small number of format directives, and therefore it is irrelevant
whether they continue the processing after an earlier format directive
failed.
Based on 1) and 2), I am opposed to this patch. The optimization is not
worth the compilation errors in the existing callers of this function
and the potential bugs caused in the future callers of this function.
Bruno
[1]
https://sources.debian.org/src/tar/1.35+dfsg-3.1/src/checkpoint.c?hl=335#L335
[2] https://codesearch.debian.net/show?file=rush_2.4-1%2Flib%2Frushdb.c&line=410
