[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 36/42] qapi: QMP interface for blkdebug and blkve
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PULL 36/42] qapi: QMP interface for blkdebug and blkverify |
Date: |
Thu, 16 Jan 2014 11:03:26 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 15.01.2014 um 16:19 hat Eric Blake geschrieben:
> On 01/15/2014 03:22 AM, Kevin Wolf wrote:
> > From: Max Reitz <address@hidden>
> >
> > Add structures to support blkdebug and blkverify in blockdev-add.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> > 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
pgprYlZfnECQL.pgp
Description: PGP signature
- [Qemu-devel] [PULL 29/42] block: Use bdrv_open_image() in bdrv_open(), (continued)
- [Qemu-devel] [PULL 29/42] block: Use bdrv_open_image() in bdrv_open(), Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 31/42] blockdev: Move "file" to legacy_opts, Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 32/42] blkdebug: Allow command-line file configuration, Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 30/42] block: Allow recursive "file"s, Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 33/42] blkverify: Allow command-line configuration, Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 35/42] qapi: Add "errno" to the list of polluted words, Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 34/42] blkverify: Don't require protocol filename, Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 36/42] qapi: QMP interface for blkdebug and blkverify, Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 37/42] qemu-io: Make filename optional, Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 39/42] tests: Add test for qdict_flatten(), Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 40/42] iotests: Test new blkdebug/blkverify interface, Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 38/42] tests: Add test for qdict_array_split(), Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 41/42] iotests: Test file format nesting, Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 42/42] block: fix backing file segfault, Kevin Wolf, 2014/01/15
- [Qemu-devel] [PULL 27/42] block: Allow block devices without files, Kevin Wolf, 2014/01/15