| Message ID | 1389781375-11774-37-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Il 15/01/2014 16:19, Eric Blake ha scritto: >> +# +# @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. This has been on my todo list for a huge time. I'll try to set aside some time. Paolo -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJS1sFDAAoJEBvWZb6bTYbycoIQAIB2bonsqfY2jcr+weibAotc 26t8+JY9jcQTs392+7naoHYLxgGzqVsas18tZO4rdX+8/qyhKxNT1gY5IsAmBvmV raI3pU69QJfvEWgjpGY5GeYMxRlVH4Pxw+XOaJARoghMx2IpeHiPqFU7Kkc1YwJ+ reFysBNEXaAheOopeWI2xKz8r+gVNeiKDMDoQ7yOpGKp5t9p/qBSLhSjL/utGxOQ rx1lSmGExRiG5wQF8rxOA2JDRYpJzGAGi2aYHP+vJDleaYAzEoHnbm5DUVEKs5ro 6vefj/Es5T3YKOfJAxtQfq3PQ62CTfkX/+hJD451moAlHLlc8WPzNfDQYm0iD6Bv M4GbSjDZB6rU/ngHcxO462VQqd9sl2WbdDeE0hdWkL2ZzVcR+SJ+Uf50FXAHCna0 hRWqES+k6Bl6wcHoCt8yF9UkBS8Ac6/puJzHl2jQT1GK8HC0+at9UU+5lArPo3kj Nsq930WjOmjeYQrH7qsn96ecBBTW/hD+PoYz/er5xjdmUNXiG3OKr/0Q3uTalyKE PNZAzFmLBx0GalmiTLySwAxB0xx+to0wl4+qwhNfSenBsM2n4Mv0YHdKFEVND40I WNVvJigkjb9lSzpdXJvoIMd860F1SHDhRf4+fjOI8pneWmZKmfNBeobtPUntyVY7 Z2HaUHDIGo6qKqvQS6zQ =p34S -----END PGP SIGNATURE-----
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
Am 16.01.2014 11:03, schrieb Kevin Wolf: > 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. [snip] FWIW for -cpu we added code to do an automatic conversion from '_' to '-' so that we could name QOM properties the new way while keeping command line compatibility. As a side-effect, both became possible IIUC. Andreas
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', + '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' ] } + +## +# @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" +# +# @errno: #optional error identifier (errno) to be returned; defaults to +# EIO +# +# @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" +# +# @new_state: the state identifier blkdebug is supposed to assume if +# this event is triggered +# +# Since: 2.0 +## +{ 'type': 'BlkdebugSetStateOptions', + 'data': { 'event': 'BlkdebugEvent', + '*state': 'int', + 'new_state': 'int' } } + +## +# @BlockdevOptionsBlkdebug +# +# Driver specific block device options for blkdebug. +# +# @image: underlying raw block device (or image file) +# +# @config: #optional filename of the configuration file +# +# @inject-error: #optional array of error injection descriptions +# +# @set-state: #optional array of state-change descriptions +# +# Since: 2.0 +## +{ 'type': 'BlockdevOptionsBlkdebug', + 'data': { 'image': 'BlockdevRef', + '*config': 'str', + '*inject-error': ['BlkdebugInjectErrorOptions'], + '*set-state': ['BlkdebugSetStateOptions'] } } + +## +# @BlockdevOptionsBlkverify +# +# Driver specific block device options for blkverify. +# +# @test: block device to be tested +# +# @raw: raw image used for verification +# +# Since: 2.0 +## +{ 'type': 'BlockdevOptionsBlkverify', + 'data': { 'test': 'BlockdevRef', + 'raw': 'BlockdevRef' } } + +## # @BlockdevOptions # # Options for creating a block device. @@ -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', 'bochs': 'BlockdevOptionsGenericFormat', 'cloop': 'BlockdevOptionsGenericFormat', 'cow': 'BlockdevOptionsGenericCOWFormat',