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 <afa...@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 <hre...@redhat.com>
