Re: [Qemu-devel] [PATCH 04/17] blockdev: 'blockdev-add' QMP command

[Eric Blake]

On 09/20/2013 05:54 AM, Kevin Wolf wrote:
> For examples see the changes to qmp-commands.hx.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  blockdev.c       |  57 ++++++++++++
>  qapi-schema.json | 270 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |  59 ++++++++++++
>  3 files changed, 386 insertions(+)
> 


> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsBase',
> +  'data': { 'driver': 'str',
> +            '*id': 'str',
> +            '*discard': 'BlockdevDiscardOptions',
> +            '*cache': 'BlockdevCacheOptions',
> +            '*aio':  'BlockdevAIOOptions',

Is the double space intentional?  Harmless, but inconsistent.

> +            '*rerror': 'BlockdevOnError',
> +            '*werror': 'BlockdevOnError',
> +            '*throttling': 'BlockdevThrottlingOptions',
> +            '*read-only': 'bool' } }
...

> +##
> +# @BlockdevOptionsVVFAT
> +#
> +# Driver specific block device options for the vvfat protocol.
> +#
> +# @dir:         directory to be exported as FAT image
> +# @fat-type:    #optional FAT type: 12, 16 or 32
> +# @floppy:      #optional whether to export a floppy imae (true) or 
> partitioned
> +#               hard disk (false; default)
> +# @rw:          #optional whether to allow write operations (default: false)

Why do we have 'read-only' in base, and 'rw' in vvfat?  It feels like
the vvfat option is redundant.

> +##
> +# @BlockdevOptionsGenericCOWFormat
> +#
> +# Driver specific block device options for image format that have no option
> +# besides their data source and an optional backing file.
> +#
> +# @file:        reference to or definition of the data source block device

Do you need to document this field...

> +# @backing:     #optional reference to or definition of the backing file 
> block
> +#               device (if missing, taken from the image file content). It is
> +#               allowed to pass an empty string here in order to disable the
> +#               default backing file.
> +# @copy-on-read: #optional whether to enable copy on read for the device
> +#                (default: false). Copy on read can only be used if the
> +#                image is not read-only.
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevOptionsGenericCOWFormat',
> +  'base': 'BlockdevOptionsGenericFormat',
> +  'data': { '*backing': 'BlockdevRef',
> +            '*copy-on-read': 'bool' } }

...given that it is only present by inheritence?

> +##
> +# @BlockdevOptionsQcow2
> +#
> +# Driver specific block device options for qcow2.
> +#
> +# @file:        reference to or definition of the data source block device
> +#
> +# @backing:     #optional reference to or definition of the backing file 
> block
> +#               device (if missing, taken from the image file content)

Same question.  If you DO document inherited fields, you missed
@copy-on-read; if you DON'T document inherited fields, these two aren't
necessary (instead, you could just have some statement about: "In
addition to the fields documented in BlockdevOptionsGenericCOWFormat,
this struct includes:")

> +##
> +# @blockdev-add:
> +#
> +# Creates a new block device.
> +#
> +# @options: block device options for the new device
> +#
> +# Since: 1.7
> +##
> +{ 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }

So deceptively simple :)  All the real work is in the rest of the
(cool!) enhancements to the qapi type generator.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature