Re: [Qemu-devel] [PATCH 2/4] block: vdi - use block layer ops in vdi_create, instead of posix calls

[Max Reitz]

On 18.07.2014 22:53, Jeff Cody wrote:
> Use the block layer to create, and write to, the image file in the
> VDI .bdrv_create() operation.
>
> This has a couple of benefits: Images can now be created over protocols,
> and host raw file optimizations (such as nocow) do not need to be
> handled in the image format driver.
>
> Also some minor cleanup for error handling.
>
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>   block/vdi.c | 68 ++++++++++++++++++++++++++-----------------------------------
>   1 file changed, 29 insertions(+), 39 deletions(-)

With this, I think you could have dropped the #ifdef __linux__ block 
from the top of block/vdi.c, too.

> diff --git a/block/vdi.c b/block/vdi.c
> index 197bd77..a74ba85 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -681,7 +681,6 @@ static int vdi_co_write(BlockDriverState *bs,
>   
>   static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
>   {
> -    int fd;
>       int result = 0;
>       uint64_t bytes = 0;
>       uint32_t blocks;
> @@ -690,7 +689,10 @@ static int vdi_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>       VdiHeader header;
>       size_t i;
>       size_t bmap_size;
> -    bool nocow = false;
> +    int64_t offset = 0;
> +    Error *local_err = NULL;
> +    BlockDriverState *bs = NULL;
> +    uint32_t *bmap = NULL;
>   
>       logout("\n");
>   
> @@ -707,7 +709,6 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
>           image_type = VDI_TYPE_STATIC;
>       }
>   #endif
> -    nocow = qemu_opt_get_bool_del(opts, BLOCK_OPT_NOCOW, false);
>   
>       if (bytes > VDI_DISK_SIZE_MAX) {
>           result = -ENOTSUP;
> @@ -717,27 +718,16 @@ static int vdi_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>           goto exit;
>       }
>   
> -    fd = qemu_open(filename,
> -                   O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> -                   0644);
> -    if (fd < 0) {
> -        result = -errno;
> +    result = bdrv_create_file(filename, opts, &local_err);
> +    if (result < 0) {
> +        error_propagate(errp, local_err);
>           goto exit;
>       }
> -
> -    if (nocow) {
> -#ifdef __linux__
> -        /* Set NOCOW flag to solve performance issue on fs like btrfs.
> -         * This is an optimisation. The FS_IOC_SETFLAGS ioctl return value will
> -         * be ignored since any failure of this operation should not block the
> -         * left work.
> -         */
> -        int attr;
> -        if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) {
> -            attr |= FS_NOCOW_FL;
> -            ioctl(fd, FS_IOC_SETFLAGS, &attr);
> -        }
> -#endif
> +    result = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | 
> BDRV_O_PROTOCOL,
> +                       NULL, &local_err);
> +    if (result < 0) {
> +        error_propagate(errp, local_err);
> +        goto exit;
>       }
>   
>       /* We need enough blocks to store the given disk size,
> @@ -769,13 +759,15 @@ static int vdi_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>       vdi_header_print(&header);
>   #endif
>       vdi_header_to_le(&header);
> -    if (write(fd, &header, sizeof(header)) < 0) {
> -        result = -errno;
> -        goto close_and_exit;
> +    result = bdrv_pwrite_sync(bs, offset, &header, sizeof(header));
> +    if (result < 0) {
> +        error_setg(errp, "Error writing header to %s", filename);
> +        goto exit;
>       }
> +    offset += sizeof(header);
>   
>       if (bmap_size > 0) {
> -        uint32_t *bmap = g_malloc0(bmap_size);
> +        bmap = g_malloc0(bmap_size);
>           for (i = 0; i < blocks; i++) {
>               if (image_type == VDI_TYPE_STATIC) {
>                   bmap[i] = i;
> @@ -783,27 +775,25 @@ static int vdi_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>                   bmap[i] = VDI_UNALLOCATED;
>               }
>           }
> -        if (write(fd, bmap, bmap_size) < 0) {
> -            result = -errno;
> -            g_free(bmap);
> -            goto close_and_exit;
> +        result = bdrv_pwrite_sync(bs, offset, bmap, bmap_size);
> +        if (result < 0) {
> +            error_setg(errp, "Error writing bmap to %s", filename);
> +            goto exit;
>           }
> -        g_free(bmap);
> +        offset += bmap_size;
>       }
>   
>       if (image_type == VDI_TYPE_STATIC) {
> -        if (ftruncate(fd, sizeof(header) + bmap_size + blocks * block_size)) {
> -            result = -errno;
> -            goto close_and_exit;
> +        result = bdrv_truncate(bs->file, offset + blocks * block_size);

bs actually *is* your file, so this should be just bs and not bs->file 
(like this, "qemu-img create -f vdi -o static=on foo.vdi 128K" segfaults).

Other than that, the patch looks good.

Max