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

[Eric Blake <eblake@redhat.com>]

On 02/21/2018 04:29 AM, Kevin Wolf wrote:

>>> +++ 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?
> 
> Anything specific you want to see here?
> 
> Essentially the meaning of BlockdevCreateOptions depends on the driver
> and is documented in the QAPI schema, how Error works is common
> knowledge, and I don't see much else to explain here.
> 
> I mean, I can add something like "Creates an image. See the QAPI
> documentation for BlockdevCreateOptions for details." if you think this
> is useful. But is it?

I guess my concern is whether this interface MUST overwrite any existing 
data in order to convert existing storage into a newly-created image of 
this driver's type (even if the overwritten data previously probed as a 
different image type), or if it is only called at a point when any 
existing data would be probed as raw, or any other useful tidbits that a 
driver might need to know in implementing it.  But if all you can think 
of is "See QAPI for BlockdevCreateOptions for details", then yeah, 
that's not worth a comment.

> 
>>> +    /* 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?
> 
> Hm, it would be more correct. On the other hand, I'm almost sure we'd
> forget to rename it back when we remove the x- prefix...

Good point.  And being an x- prefix implies that inconsistency may be 
expected (not to mention short-lived, if we promote the interface); 
while being consistent now but risking long-term inconsistency down the 
road when it actually becomes a stable interface is indeed worse.  So 
keep this message as-is.

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