Re: [Qemu-devel] [PATCH 3/7] block/raw-posix: refactor handle_aiocb_write_zeroes a bit

[Max Reitz <mreitz@redhat.com>]

On 2015-01-27 at 08:51, Denis V. Lunev wrote:
> move code dealing with a block device to a separate function. This will
> allow to implement additional processing for ordinary files.
>
> Pls note, that xfs_code has been moved before checking for

Please use "Please". :-)

> s->has_write_zeroes as xfs_write_zeroes does not touch this flag inside.
> This makes code a bit more consistent.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Peter Lieven <pl@kamp.de>
> CC: Fam Zheng <famz@redhat.com>
> ---
>   block/raw-posix.c | 48 +++++++++++++++++++++++++++++-------------------
>   1 file changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 2aa268a..24e1fab 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -914,41 +914,51 @@ static int do_fallocate(int fd, int mode, off_t offset, off_t len)
>   }
>   #endif
>   
> -static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
> +static ssize_t handle_aiocb_write_zeroes_block(RawPosixAIOData *aiocb)
>   {
> -    int ret = -EOPNOTSUPP;
> +    int ret = -ENOTSUP;
>       BDRVRawState *s = aiocb->bs->opaque;
>   
> -    if (s->has_write_zeroes == 0) {
> +    if (!s->has_write_zeroes) {
>           return -ENOTSUP;
>       }
>   
> -    if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
>   #ifdef BLKZEROOUT
> -        do {
> -            uint64_t range[2] = { aiocb->aio_offset, aiocb->aio_nbytes };
> -            if (ioctl(aiocb->aio_fildes, BLKZEROOUT, range) == 0) {
> -                return 0;
> -            }
> -        } while (errno == EINTR);
> -
> -        ret = -errno;
> -#endif
> -    } else {
> -#ifdef CONFIG_XFS
> -        if (s->is_xfs) {
> -            return xfs_write_zeroes(s, aiocb->aio_offset, aiocb->aio_nbytes);
> +    do {
> +        uint64_t range[2] = { aiocb->aio_offset, aiocb->aio_nbytes };
> +        if (ioctl(aiocb->aio_fildes, BLKZEROOUT, range) == 0) {
> +            return 0;
>           }
> +    } while (errno == EINTR);
> +
> +    ret = translate_err(-errno);
>   #endif
> -    }
>   
> -    ret = translate_err(ret);
>       if (ret == -ENOTSUP) {
>           s->has_write_zeroes = false;
>       }
>       return ret;
>   }
>   
> +static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
> +{
> +    int ret = -ENOTSUP;
> +    BDRVRawState *s = aiocb->bs->opaque;
> +
> +    if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
> +        return handle_aiocb_write_zeroes_block(aiocb);
> +    }
> +
> +#ifdef CONFIG_XFS
> +    if (s->is_xfs) {
> +        return xfs_write_zeroes(s, aiocb->aio_offset, aiocb->aio_nbytes);
> +    }
> +#endif
> +
> +    s->has_write_zeroes = false;
> +    return ret;
> +}
> +

It'll probably look nicer if you remove the "ret" variable from this 
function completely and just "return -ENOTSUP" at the end.

With s/Pls/Please/ in the commit message and with or without "ret" 
removed from this function:

Reviewed-by: Max Reitz <mreitz@redhat.com>