Re: BufFileRead() error signalling

On Thu, May 28, 2020 at 7:10 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> On Wed, May 27, 2020 at 11:59:59AM -0400, Alvaro Herrera wrote:
> > In the discussion that led to 811b6e36a9e2 I did suggest to use "read
> > only M of N" instead, but there wasn't enough discussion on that fine
> > point so we settled on what you now call prevalent without a lot of
> > support specifically on that. I guess it was enough of an improvement
> > over what was there. But like Robert, I too prefer the wording that
> > includes "only" and "bytes" over the wording that doesn't.
> >
> > I'll let it be known that from a translator's point of view, it's a
> > ten-seconds job to update a fuzzy string from not including "only" and
> > "bytes" to one that does. So let's not make that an argument for not
> > changing.
>
> Using "only" would be fine by me, though I tend to prefer the existing
> one. Now I think that we should avoid "bytes" to not have to worry
> about pluralization of error messages. This has been a concern in the
> past (see e5d11b9 and the likes).

Sorry for the delay in producing a new patch. Here's one that tries
to take into account the various feedback in this thread:

Ibrar said:
> You are checking file->dirty twice, first before calling the function
> and within the function too. Same for the Assert.

Fixed.

Robert, Melanie, Michael and Alvaro put forward various arguments
about the form of "short read" and block seek error message. While
removing bogus cases of "%m", I changed them all to say "read only %zu
of %zu bytes" in the same place. I removed the bogus "%m" from
BufFileSeek() and BufFileSeekBlock() callers (its call to
BufFileFlush() already throws). I added block numbers to the error
messages about failure to seek by block.

Following existing practice, I made write failure assume ENOSPC if
errno is 0, so that %m would make sense (I am not sure what platform
reports out-of-space that way, but there are certainly a lot of copies
of that code in the tree so it seemed to be missing here).

I didn't change BufFileWrite() to be void, to be friendly to existing
callers outside the tree (if there are any), though I removed all the
code that checks the return code. We can make it void later.

For the future: it feels a bit like we're missing a one line way to
say "read this many bytes and error out if you run out".