Re: [PATCH v5 06/10] block: Make 'bytes' param of bdrv_co_{pread,pwrite,preadv,pwritev}() an int64_t

[Hanna Reitz]

On 09.06.22 17:27, Alberto Faria wrote:
> For consistency with other I/O functions, and in preparation to
> implement bdrv_{pread,pwrite}() using generated_co_wrapper.
>
> unsigned int fits in int64_t, so all callers remain correct.
>
> bdrv_check_request32() is called further down the stack and causes -EIO
> to be returned if 'bytes' is negative or greater than
> BDRV_REQUEST_MAX_BYTES, which in turns never exceeds SIZE_MAX.

I’m not a huge fan of that reasoning alone.  I don’t like generating an 
object that will be invalid if `bytes > SIZE_MAX`, and then rely on some 
later check in a different context verifying that `bytes <= SIZE_MAX`.  
In theory, if the latter check is removed, we might forget caring for 
the former.  (In practice, such a case (where I/O vectors remain using 
size_t, but we allow for larger overall requests) is difficult to 
imagine, though.)

However, bdrv_check_request32() also calls bdrv_check_qiov_request(), 
which verifies the integrity of qiov by checking that `bytes` will not 
exceed `qiov->size - qiov_offset`.  So if we had any overflow when 
casting `bytes` to `size_t`, it’ll be seen there directly (and I don’t 
see why we’d remove that specific check).

Given that, and given that there’s precedent (e.g. bdrv_pread()), I’m OK 
with the change.

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