Re: [PATCH v5 07/10] block: Implement bdrv_{pread,pwrite,pwrite_zeroes}() using generated_co_wrapper

[Hanna Reitz]

On 09.06.22 17:27, Alberto Faria wrote:
> bdrv_{pread,pwrite}() now return -EIO instead of -EINVAL when 'bytes' is
> negative, making them consistent with bdrv_{preadv,pwritev}() and
> bdrv_co_{pread,pwrite,preadv,pwritev}().
>
> bdrv_pwrite_zeroes() now also calls trace_bdrv_co_pwrite_zeroes() and
> clears the BDRV_REQ_MAY_UNMAP flag when appropriate, which it didn't
> previously.
>
> Signed-off-by: Alberto Faria <afaria@redhat.com>
> ---
>
> I audited all bdrv_{pread,pwrite}() callers to make sure that changing
> the -EINVAL return code to -EIO wont't break things. However, there are
> about 140 call sites, so the probability of me having missed something
> isn't negligible. If someone more accustomed to the code base is able to
> double-check this, that would be very much appreciated.

FWIW I checked all call sits when reviewing patch 3, and I can’t 
remember any case where the follow-up check was anything but `ret < 0`.  
The only difference should be a couple of error_setg_errno() calls, 
which will change from “Invalid argument” to “I/O error”.

I guess the real problem wouldn’t be checking the immediate call sites, 
but the fact that most call sites just pass the error code to their 
caller in turn, and that’s really something that’s unreasonable to 
check, I believe.

Honestly, I don’t think it really matters (how likely is `bytes < 0`?), 
nor could I imagine a case where EINVAL vs. EIO would cause any 
difference in behavior.  That’s to say, I’d be disappointed if it did.

(Grepping for 'if.*EINVAL' and 'if.*EIO' in block/ only yields one case 
in block/nbd.c where I can’t quickly absolutely rule out it won’t make a 
difference, but I think that check is only for error codes coming from 
handling network requests.)

> As a precaution, I also dropped Paolo's R-b.
>
>   block/io.c               | 41 ----------------------------------------
>   include/block/block-io.h | 15 +++++++++------
>   2 files changed, 9 insertions(+), 47 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>