Re: [Qemu-devel] [PULL 36/42] qapi: QMP interface for blkdebug and blkverify

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4424 bytes --]

On 01/15/2014 03:22 AM, Kevin Wolf wrote:
> From: Max Reitz <mreitz@redhat.com>
> 
> Add structures to support blkdebug and blkverify in blockdev-add.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi-schema.json | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 109 insertions(+), 4 deletions(-)

Sorry for not noticing this sooner, but we still have some interface
problems that need to be ironed out.

> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f27c48a..35f7b34 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4201,6 +4201,113 @@
>              '*pass-discard-other': 'bool' } }
>  
>  ##
> +# @BlkdebugEvent
> +#
> +# Trigger events supported by blkdebug.
> +##
> +{ 'enum': 'BlkdebugEvent',

Missing a 'Since: 2.0' designation; would be worth adding that in a
followup patch.

> +  'data': [ 'l1_update', 'l1_grow.alloc_table', 'l1_grow.write_table',
> +            'l1_grow.activate_table', 'l2_load', 'l2_update',
> +            'l2_update_compressed', 'l2_alloc.cow_read', 'l2_alloc.write',
> +            'read_aio', 'read_backing_aio', 'read_compressed', 'write_aio',
> +            'write_compressed', 'vmstate_load', 'vmstate_save', 'cow_read',
> +            'cow_write', 'reftable_load', 'reftable_grow', 'reftable_update',
> +            'refblock_load', 'refblock_update', 'refblock_update_part',
> +            'refblock_alloc', 'refblock_alloc.hookup', 'refblock_alloc.write',
> +            'refblock_alloc.write_blocks', 'refblock_alloc.write_table',
> +            'refblock_alloc.switch_table', 'cluster_alloc',
> +            'cluster_alloc_bytes', 'cluster_free', 'flush_to_os',
> +            'flush_to_disk' ] }

Do we want to prefer '-' over '_' in all these names?

> +
> +##
> +# @BlkdebugInjectErrorOptions
> +#
> +# Describes a single error injection for blkdebug.
> +#
> +# @event:       trigger event
> +#
> +# @state:       #optional the state identifier blkdebug needs to be in to
> +#               actually trigger the event; defaults to "any"

This sounds like it is a finite set of states, and therefore should be
an enum rather than an 'int'.

> +#
> +# @errno:       #optional error identifier (errno) to be returned; defaults to
> +#               EIO

errno is not a portable.  The values of errno on one machine may not
match the values of errno on the other machine using the QMP connection
via a remote socket.  It might be cleaner to have an enum of named error
values, rather than integer errno values.

> +#
> +# @sector:      #optional specifies the sector index which has to be affected
> +#               in order to actually trigger the event; defaults to "any
> +#               sector"
> +#
> +# @once:        #optional disables further events after this one has been
> +#               triggered; defaults to false
> +#
> +# @immediately: #optional fail immediately; defaults to false
> +#
> +# Since: 2.0
> +##
> +{ 'type': 'BlkdebugInjectErrorOptions',
> +  'data': { 'event': 'BlkdebugEvent',
> +            '*state': 'int',
> +            '*errno': 'int',
> +            '*sector': 'int',
> +            '*once': 'bool',
> +            '*immediately': 'bool' } }
> +
> +##
> +# @BlkdebugSetStateOptions
> +#
> +# Describes a single state-change event for blkdebug.
> +#
> +# @event:       trigger event
> +#
> +# @state:       #optional the current state identifier blkdebug needs to be in;
> +#               defaults to "any"
> +#

Again, this should be an enum, not an int.

> +# @new_state:   the state identifier blkdebug is supposed to assume if
> +#               this event is triggered

s/new_state/new-state/

> @@ -4224,10 +4331,8 @@
>  # TODO sheepdog: Wait for structured options
>  # TODO ssh: Should take InetSocketAddress for 'host'?
>        'vvfat':      'BlockdevOptionsVVFAT',
> -
> -# TODO blkdebug: Wait for structured options
> -# TODO blkverify: Wait for structured options
> -
> +      'blkdebug':   'BlockdevOptionsBlkdebug',
> +      'blkverify':  'BlockdevOptionsBlkverify',

As we are adding new arms to the union, we should document that these
values are available since 2.0.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]