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

[Kevin Wolf <kwolf@redhat.com>]

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

Am 15.01.2014 um 16:19 hat Eric Blake geschrieben:
> 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.

Nothing that should hold up this pull request, there are just a few
improvements that can be done in a follow-up.

> > 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.

Ack.

> > +  '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?

These are existing names from the blkdebug configuration file. If we
wanted to have '-' in QMP and '_' in the config file, we'd have to have
some conversion somewhere. Uglier than having underscores here, imho.

> > +
> > +##
> > +# @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'.

No, these are user-defined states.

> > +#
> > +# @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.

That's true. On the other hand this is only blkdebug, which is used for
testing and debugging and nothing else. I know what the errno values are
on my machine, so in practice this is good enough for me.

If someone feels bored and wants to clean it up later, be my guest, but
as far as I am concerned, in this specific place it's a non-problem.

> > +#
> > +# @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.

See above.

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

Again, existing blkdebug config stuff.

> > @@ -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.

Hm, yes, I think this request makes sense. Do we have an existing example
of such documentation? Because I'm not quite sure where to put it.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]