Correct the use of ::read()

[tobim]

No description provided.

[dominiklohmann]

I did not run this on my machine, since it is very hard to test for this case, but from looking at the code and the man page this seems to be correct.

[mavam]

I'm a bit on the fence. There are two changes here (which would have been nice to make explicit, either via commit message or comment):

1. Do not fail when read(2) returns 0.
2. Busy-loop until all requested bytes are fetched.

Is this correct?

[dominiklohmann]

• Do not fail when read(2) returns 0.

Maybe we need a check for total != 0 at the end of the function?

[mavam]

There seems to be a bug though: you are overwriting the same buffer multiple times instead of advancing the pointer by taken.

[mavam]

It looks correct now. However, the fact that the first review didn't catch this immediately indicates that we need a more objective way to verify the functionality. How would you suggest we test this? A unit test that reads a large file?

[mavam]

I think the function signature is not good, actually. It should return expected<size_t> and not use an optional got output argument. The caller should easily determine whether a partial read occurs. This would be ideal:

auto xs = span<byte>{...};
auto n = read(xs);
if (!n)
  // failure
else if (*n != xs.size())
  // partial read

The span migration is too heavy-weight for this PR, and we should apply it uniformly to all related functions, but I think the return value should be changed now that we're touching this function.

[mavam]

Why are we changing the implementation of the POSIX wrappers so dramatically with the latest changes? Wasn't it almost working already? fread is a buffered libc function, whereas read is just a syscall, no? But we provide the buffer, so now we have two buffers in the mix, one managed by fread and the other being our application-level buffer.

[mavam]

In CI or in general?

[dominiklohmann]

Or rather: With HFS oder APFS?

[tobim]

In general with APFS on my office iMac.

[tobim]

@mavam As discussed I figured out how to use the level 2 API correctly on macOS. The code can now be reviewed again.

[mavam]

I like the current solution.

One last thing: please add a changelog entry about the bugfix since this was a user-reported error.

[lava]

I hate to be piling on here, but imho read/write are not good names for these functions. There is already a well-known function called read(int, void*, size_t) that everyone is familiar with, and from looking at the name and signature I'd expect a function called posix::read() to be just a thing wrapper around that. As is, everybody glancing at that will assume there is a bug in the calling code because we dont retry in case of a partial read.

I dont mind committing this now to get the bugfix out to DCSO, but imho renaming this to something like read_all()/safe_read()/read_n()/etc. (that can maybe call the actual read internally) should be the near-term goal.

[lava]

Addendum to the above: I just realized that the API change is bigger than I initially thought. Usually one passes an upper bound to read(), i.e. the size of the buffer. After the changes here, the size also becomes a lower bound, i.e. one has to know exactly how many bytes are to be read in order to call this function.

[tobim]

After the changes here, the size also becomes a lower bound, i.e. one has to know exactly how many bytes are to be read in order to call this function.

That is not the case, see https://github.com/tenzir/vast/pull/1025/files#diff-4d6b0fc0d187a8a24760ed369528e14fR272-R273.

The requested changes have been applied