Re: [Qemu-devel] [PATCH 11/27] block: x-blockdev-create QMP command

[Eric Blake <eblake@redhat.com>]

On 02/08/2018 01:23 PM, Kevin Wolf wrote:
> This adds a synchronous x-blockdev-create QMP command that can create
> qcow2 images on a given node name.
> 
> We don't want to block while creating an image, so this is not the final
> interface in all aspects, but BlockdevCreateOptionsQcow2 and
> .bdrv_co_create() are what they actually might look like in the end. In
> any case, this should be good enough to test whether we interpret
> BlockdevCreateOptions as we should.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

> @@ -3442,6 +3442,18 @@
>     } }
>   
>   ##
> +# @x-blockdev-create:
> +#
> +# Create an image format on a given node.
> +# TODO Replace with something asynchronous (block job?)

We've learned our lesson - don't commit to the final name on an 
interface that we have not yet experimented with.

> +#
> +# Since: 2.12
> +##
> +{ 'command': 'x-blockdev-create',
> +  'data': 'BlockdevCreateOptions',
> +  'boxed': true }
> +

Lots of code packed in that little description ;)


> +++ b/include/block/block_int.h
> @@ -130,6 +130,8 @@ struct BlockDriver {
>       int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
>                             Error **errp);
>       void (*bdrv_close)(BlockDriverState *bs);
> +    int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
> +                                       Error **errp);

I know we haven't been very good in the past, but can you add a comment 
here on the contract that drivers are supposed to obey when implementing 
this callback?

> +++ b/block.c
> @@ -369,7 +369,7 @@ BlockDriver *bdrv_find_format(const char *format_name)
>       return bdrv_do_find_format(format_name);
>   }
>   
> -static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
> +int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)

Worth mentioning that bdrv_is_whitelisted had to be exported as part of 
the commit message?  (Or even promoting it to public in a separate commit?)

> +++ b/block/create.c
> @@ -0,0 +1,75 @@
> +/*
> + * Block layer code related to image creation
> + *
> + * Copyright (c) 2018 Kevin Wolf <kwolf@redhat.com>

The question came up in another thread, but I didn't hear your answer 
there; I know Red Hat permits you to claim personal copyright while 
still using a redhat.com address for code written on personal time, but 
should this claim belong to Red Hat instead of you?

> +    /* Call callback if it exists */
> +    if (!drv->bdrv_co_create) {
> +        error_setg(errp, "Driver does not support blockdev-create");

Should this error message refer to 'x-blockdev-create' in the short term?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org