From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54277) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W3jnb-0002ei-Ls for qemu-devel@nongnu.org; Thu, 16 Jan 2014 05:03:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W3jnO-0002KL-VZ for qemu-devel@nongnu.org; Thu, 16 Jan 2014 05:03:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:9115) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W3jnO-0002KB-NU for qemu-devel@nongnu.org; Thu, 16 Jan 2014 05:03:30 -0500 Date: Thu, 16 Jan 2014 11:03:26 +0100 From: Kevin Wolf Message-ID: <20140116100326.GC3369@dhcp-200-207.str.redhat.com> References: <1389781375-11774-1-git-send-email-kwolf@redhat.com> <1389781375-11774-37-git-send-email-kwolf@redhat.com> <52D6A6E5.8080407@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="LZvS9be/3tNcYl/X" Content-Disposition: inline In-Reply-To: <52D6A6E5.8080407@redhat.com> Subject: Re: [Qemu-devel] [PULL 36/42] qapi: QMP interface for blkdebug and blkverify List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, anthony@codemonkey.ws --LZvS9be/3tNcYl/X Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 15.01.2014 um 16:19 hat Eric Blake geschrieben: > On 01/15/2014 03:22 AM, Kevin Wolf wrote: > > From: Max Reitz > >=20 > > Add structures to support blkdebug and blkverify in blockdev-add. > >=20 > > Signed-off-by: Max Reitz > > Signed-off-by: Kevin Wolf > > --- > > qapi-schema.json | 113 +++++++++++++++++++++++++++++++++++++++++++++++= ++++++-- > > 1 file changed, 109 insertions(+), 4 deletions(-) >=20 > 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' } } > > =20 > > ## > > +# @BlkdebugEvent > > +# > > +# Trigger events supported by blkdebug. > > +## > > +{ 'enum': 'BlkdebugEvent', >=20 > 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.wri= te', > > + 'read_aio', 'read_backing_aio', 'read_compressed', 'write_= aio', > > + 'write_compressed', 'vmstate_load', 'vmstate_save', 'cow_r= ead', > > + 'cow_write', 'reftable_load', 'reftable_grow', 'reftable_u= pdate', > > + 'refblock_load', 'refblock_update', 'refblock_update_part', > > + 'refblock_alloc', 'refblock_alloc.hookup', 'refblock_alloc= =2Ewrite', > > + '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' ] } >=20 > 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" >=20 > 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; def= aults to > > +# EIO >=20 > 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 a= ffected > > +# in order to actually trigger the event; defaults to "a= ny > > +# sector" > > +# > > +# @once: #optional disables further events after this one has b= een > > +# 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" > > +# >=20 > 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 >=20 > 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', >=20 > 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 --LZvS9be/3tNcYl/X Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJS165uAAoJEH8JsnLIjy/Wk8AQALn8EPV+bv53JAjsleFyIrnB JyK1xJd6mQFj9axsziCe/m99/sG8QSSfKqrxCDUBCJ+mHc374Oa17drdUJRxwsts CVJHRNNZLzoNk2dE9Xue/tdyYERthJDCRnix2GwzK2m/IpawKvm+3ljNNHbLu94s XQZv4vC2+gXlDpQmmSIM5a93qtjGSvKihRRMgyYLdIXJKbig1Hh6udg8ZfvZrjxB lmBLGXGQwuCzXxcGa5Dnx7C0NmutXFKh666sszF/dPd6r0gnEB5peCVP+ZcYjhGx HOPVKD/QQlFO+xvfxuJcEDMgc4atXPjFJ9oZXkSBBp7mcUxv12F3ZZ37v6ODv5nx A/hB+lmawc4BFgJdPnq0q7k3zfZmNhiCFZ5Zd9yigB/m3vdu7WKmS9r3i5ZLG4J0 nU/gmbEfopr1PtD6zuTkC0Nn7xzkD8e87K0IV4051H25T6HfScq89gdOL2gKyxGc viXOv83XLwz3HpAIphYGch7AiWmvTowQ9/oLQgpuUK0fI7MWHnupqGQjPoOJ8umq VV2QyHlr+ea7nka+istPr6GXFvMlJzwx/X5c1JHy/di3HS4zxlg4Opp7heCl4rgQ i+2tEWJ7dR93I98rYEOHIqpJlklKcm/fKeT2RTWmYkYqcJRXclHVa6c86t5kQ/7e YnraYjPrzcS3EHUlZkaa =SJmc -----END PGP SIGNATURE----- --LZvS9be/3tNcYl/X--