Re: [Qemu-devel] [PATCH v2 13/24] block: Introduce bdrv_co_do_pwritev()

[Max Reitz <mreitz@redhat.com>]

On 13.12.2013 14:22, Kevin Wolf wrote:
> This is going to become the bdrv_co_do_preadv() equivalent for writes.
> In this patch, however, just a function taking byte offsets is created,
> it doesn't align anything yet.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c | 23 +++++++++++++++++------
>   1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/block.c b/block.c
> index 385fb8a..a80db2e 100644
> --- a/block.c
> +++ b/block.c
> @@ -3010,8 +3010,8 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
>   /*
>    * Handle a write request in coroutine context
>    */
> -static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
> -    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
> +static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
> +    int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
>       BdrvRequestFlags flags)
>   {
>       int ret;
> @@ -3022,21 +3022,32 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
>       if (bs->read_only) {
>           return -EACCES;
>       }
> -    if (bdrv_check_request(bs, sector_num, nb_sectors)) {
> +    if (bdrv_check_byte_request(bs, offset, bytes)) {
>           return -EIO;
>       }
>   
>       /* throttling disk I/O */
>       if (bs->io_limits_enabled) {
> -        bdrv_io_limits_intercept(bs, nb_sectors, true);
> +        bdrv_io_limits_intercept(bs, bytes << BDRV_SECTOR_BITS, true);
>       }
>   
> -    ret = bdrv_aligned_pwritev(bs, sector_num << BDRV_SECTOR_BITS,
> -                               nb_sectors << BDRV_SECTOR_BITS, qiov, flags);
> +    ret = bdrv_aligned_pwritev(bs, offset, bytes, qiov, flags);
>   
>       return ret;
>   }
>   
> +static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
> +    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
> +    BdrvRequestFlags flags)
> +{
> +    if (nb_sectors < 0 || nb_sectors > (UINT_MAX >> BDRV_SECTOR_BITS)) {

This should probably be INT_MAX, since nb_sectors is an integer. If 
nb_sectors is between INT_MAX >> BDRV_SECTOR_BITS and UINT_MAX >> 
BDRV_SECTOR_BITS, the result of nb_sectors << BDRV_SECTOR_BITS (which 
will be a signed integer) is undefined. It is obviously then implicitly 
casted to an unsigned integer (which is the type of the parameter 
"bytes") which will probably solve the problem as intended, but it is 
still technically undefined.

Thus, I'd either change this to INT_MAX or cast nb_sectors to an 
unsigned int before shifting it below.

Max

> +        return -EINVAL;
> +    }
> +
> +    return bdrv_co_do_pwritev(bs, sector_num << BDRV_SECTOR_BITS,
> +                              nb_sectors << BDRV_SECTOR_BITS, qiov, flags);
> +}
> +
>   int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
>       int nb_sectors, QEMUIOVector *qiov)
>   {