Open nh2 opened 1 month ago
Thus you need to loop around read(), always.
I am not so sure. In i3status, it's usually better to keep going to not block the program rather than potentially being stuck in a while-read loop.
The reason why the lack of short read handling is typically not a problem with i3status is that it mostly reads /proc and /sys files, for which the kernel never returns a short read.
That said, at the very least for print_file_contents.c we should consider handling short reads, and it probably doesn’t hurt for the other reads either.
@orestisfl’s point also stands, though: would handling short reads change behavior when i3status is configured to print a file that is backed by an unavailable NFS mount (for example)?
In i3status, it's usually better to keep going to not block the program rather than potentially being stuck in a while-read loop.
Short reads can be very confusing and lead to bad decisions. Imagine that this very general function slurp()
one day gets used to read a path to delete stored on disk in some config file ending in cacheDir = /home/myuser/.cache
, short-reads only cacheDir = /home/myuser
, and then runs rm -r
on that.
The reason why the lack of short read handling is typically not a problem with i3status is that it mostly reads /proc and /sys files, for which the kernel never returns a short read.
Fair point. Then the function should have a mega disclaimer at the top though when it's safe to use, probably even in the name.
That said, at the very least for print_file_contents.c we should consider handling short reads, and it probably doesn’t hurt for the other reads either.
Yes, I think that's better. If some file by implementation of the kernel needs only one call to read, that's nice and fast, but that shouldn't be assumed in general-purpose functions because it's easy to introduce future bugs.
would handling short reads change behavior when i3status is configured to print a file that is backed by an unavailable NFS mount (for example)?
I think in the general case on a hanging kernel NFS, or FUSE mount, already the first read()
will hang indefinitely (or for very long), so I doubt short-reading it would help here.
The only situation I can imagine (but am not sure it actually is implemented that way) is that you read a very large file (e.g. 1 GB), and after the first 500 MB the NFS server goes down (and clearly says so, thus avoiding a hang). Maybe then you get s short read.
From what I can tell so far, short reads don't seem to be designed to handle hangs from userspace, but to allow kernels to work with non-infinite buffers.
Imagine that this very general function
slurp()
one day gets used to read a path to delete stored on disk in some config file ending incacheDir = /home/myuser/.cache
, short-reads onlycacheDir = /home/myuser
, and then runsrm -r
on that.
As a matter of policy, i3status must never intentionally modify system state, including writing new files or deleting existing files.
I think the potential for short reads to cause issues is limited to “my i3status briefly showed a cut-off value”.
But, I also don’t want to keep arguing over this detail. Do you want to send a PR to add the loop?
The project does not check for short
read()
s.In C, as per
man 2 read
, it returns the number of Bytes read:Thus you need to loop around
read()
, always.Relevant code
https://github.com/i3/i3status/blob/200fef9e0d3663835b04e18ad067d9656b75b9cd/src/general.c#L34-L37
https://github.com/i3/i3status/blob/200fef9e0d3663835b04e18ad067d9656b75b9cd/src/print_file_contents.c#L36
This is wrong since 15 years ago: