Re: [Qemu-devel] [RFC PATCH v4 3/4] raw-posix: Add full image preallocation option

[Stefan Hajnoczi]

On Fri, Dec 27, 2013 at 11:05:53AM +0800, Hu Tao wrote:
> This patch adds a new option preallocation for raw format, and implements
> full preallocation.
> 
> Signed-off-by: Hu Tao <address@hidden>
> ---
>  block/raw-posix.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 6f6b8c1..a722d27 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1160,17 +1160,52 @@ static int64_t 
> raw_get_allocated_file_size(BlockDriverState *bs)
>      return (int64_t)st.st_blocks * 512;
>  }
>  
> +#ifdef __linux__
> +static int raw_preallocate2(int fd, int64_t offset, int64_t length)

Why is this function called raw_preallocate2()?  Please choose a
descriptive name.

> +{
> +    int ret = -1;
> +
> +    ret = fallocate(fd, 0, offset, length);
> +    if (ret < 0) {
> +        ret = -errno;
> +    }
> +
> +    /* fallback to posix_fallocate() if fallocate() is not supported */
> +    if (ret == -ENOSYS || ret == -EOPNOTSUPP) {
> +        ret = posix_fallocate(fd, offset, length);

>From the posix_fallocate(3) man page:
"posix_fallocate() returns zero on success, or an error number on
failure.  Note that errno is not set."

You need to negate the error number:
if (ret > 0) {
    ret = -ret; /* posix_fallocate(3) returns a positive errno */
}

> +    }
> +
> +    return ret;
> +}
> +#else
> +static int raw_preallocate2(int fd, int64_t offset, int64_t length)
> +{
> +    return posix_fallocate(fd, offset, length);

Same error number problem as above.

> @@ -1185,6 +1220,12 @@ static int raw_create(const char *filename, 
> QEMUOptionParameter *options,
>              result = -errno;
>              error_setg_errno(errp, -result, "Could not resize file");
>          }
> +        if (prealloc == PREALLOC_MODE_FULL) {
> +            result = raw_preallocate2(fd, 0, total_size);

What if ftruncate() failed?  We should not try to fallocate.  Please add
a proper error return path.

> +            if (result != 0)
> +                error_setg_errno(errp, -result,
> +                                 "Could not preallocate data for the new 
> file");

WARNING: braces {} are necessary for all arms of this statement

Please run scripts/checkpatch.pl before submitting patches, it checks
QEMU coding style.