From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49690) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zz017-0002u3-8j for qemu-devel@nongnu.org; Wed, 18 Nov 2015 05:31:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zz015-0002ri-AL for qemu-devel@nongnu.org; Wed, 18 Nov 2015 05:31:09 -0500 From: Markus Armbruster References: <1447836791-369-1-git-send-email-eblake@redhat.com> <1447836791-369-20-git-send-email-eblake@redhat.com> Date: Wed, 18 Nov 2015 11:30:57 +0100 In-Reply-To: <1447836791-369-20-git-send-email-eblake@redhat.com> (Eric Blake's message of "Wed, 18 Nov 2015 01:52:54 -0700") Message-ID: <874mgjvbtq.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v12 19/36] blkdebug: Merge hand-rolled and qapi BlkdebugEvent enum List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , qemu-devel@nongnu.org, "open list:Block layer core" Eric Blake writes: > No need to keep two separate enums, where editing one is likely > to forget the other. Now that we can specify a qapi enum prefix, > we don't even have to change the bulk of the uses. > > get_event_by_name() could perhaps be replaced by qapi_enum_parse(), > but I left that for another day. > > CC: Kevin Wolf > Signed-off-by: Eric Blake > > --- > v12: new patch > --- > block.c | 2 +- > block/blkdebug.c | 79 +++++++---------------------------------------- > docs/blkdebug.txt | 7 +++-- > include/block/block.h | 62 +------------------------------------ > include/block/block_int.h | 2 +- > qapi/block-core.json | 4 ++- > 6 files changed, 21 insertions(+), 135 deletions(-) > > diff --git a/block.c b/block.c > index 3a7324b..9971976 100644 > --- a/block.c > +++ b/block.c > @@ -2851,7 +2851,7 @@ ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs) > return NULL; > } > > -void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event) > +void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event) > { > if (!bs || !bs->drv || !bs->drv->bdrv_debug_event) { > return; > diff --git a/block/blkdebug.c b/block/blkdebug.c > index 6860a2b..76805a6 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -35,7 +35,7 @@ typedef struct BDRVBlkdebugState { > int state; > int new_state; > > - QLIST_HEAD(, BlkdebugRule) rules[BLKDBG_EVENT_MAX]; > + QLIST_HEAD(, BlkdebugRule) rules[BLKDBG_MAX]; > QSIMPLEQ_HEAD(, BlkdebugRule) active_rules; > QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs; > } BDRVBlkdebugState; > @@ -63,7 +63,7 @@ enum { > }; > > typedef struct BlkdebugRule { > - BlkDebugEvent event; > + BlkdebugEvent event; > int action; > int state; > union { > @@ -142,69 +142,12 @@ static QemuOptsList *config_groups[] = { > NULL > }; > > -static const char *event_names[BLKDBG_EVENT_MAX] = { > - [BLKDBG_L1_UPDATE] = "l1_update", > - [BLKDBG_L1_GROW_ALLOC_TABLE] = "l1_grow.alloc_table", > - [BLKDBG_L1_GROW_WRITE_TABLE] = "l1_grow.write_table", > - [BLKDBG_L1_GROW_ACTIVATE_TABLE] = "l1_grow.activate_table", > - > - [BLKDBG_L2_LOAD] = "l2_load", > - [BLKDBG_L2_UPDATE] = "l2_update", > - [BLKDBG_L2_UPDATE_COMPRESSED] = "l2_update_compressed", > - [BLKDBG_L2_ALLOC_COW_READ] = "l2_alloc.cow_read", > - [BLKDBG_L2_ALLOC_WRITE] = "l2_alloc.write", > - > - [BLKDBG_READ_AIO] = "read_aio", > - [BLKDBG_READ_BACKING_AIO] = "read_backing_aio", > - [BLKDBG_READ_COMPRESSED] = "read_compressed", > - > - [BLKDBG_WRITE_AIO] = "write_aio", > - [BLKDBG_WRITE_COMPRESSED] = "write_compressed", > - > - [BLKDBG_VMSTATE_LOAD] = "vmstate_load", > - [BLKDBG_VMSTATE_SAVE] = "vmstate_save", > - > - [BLKDBG_COW_READ] = "cow_read", > - [BLKDBG_COW_WRITE] = "cow_write", > - > - [BLKDBG_REFTABLE_LOAD] = "reftable_load", > - [BLKDBG_REFTABLE_GROW] = "reftable_grow", > - [BLKDBG_REFTABLE_UPDATE] = "reftable_update", > - > - [BLKDBG_REFBLOCK_LOAD] = "refblock_load", > - [BLKDBG_REFBLOCK_UPDATE] = "refblock_update", > - [BLKDBG_REFBLOCK_UPDATE_PART] = "refblock_update_part", > - [BLKDBG_REFBLOCK_ALLOC] = "refblock_alloc", > - [BLKDBG_REFBLOCK_ALLOC_HOOKUP] = "refblock_alloc.hookup", > - [BLKDBG_REFBLOCK_ALLOC_WRITE] = "refblock_alloc.write", > - [BLKDBG_REFBLOCK_ALLOC_WRITE_BLOCKS] = "refblock_alloc.write_blocks", > - [BLKDBG_REFBLOCK_ALLOC_WRITE_TABLE] = "refblock_alloc.write_table", > - [BLKDBG_REFBLOCK_ALLOC_SWITCH_TABLE] = "refblock_alloc.switch_table", > - > - [BLKDBG_CLUSTER_ALLOC] = "cluster_alloc", > - [BLKDBG_CLUSTER_ALLOC_BYTES] = "cluster_alloc_bytes", > - [BLKDBG_CLUSTER_FREE] = "cluster_free", > - > - [BLKDBG_FLUSH_TO_OS] = "flush_to_os", > - [BLKDBG_FLUSH_TO_DISK] = "flush_to_disk", > - > - [BLKDBG_PWRITEV_RMW_HEAD] = "pwritev_rmw.head", > - [BLKDBG_PWRITEV_RMW_AFTER_HEAD] = "pwritev_rmw.after_head", > - [BLKDBG_PWRITEV_RMW_TAIL] = "pwritev_rmw.tail", > - [BLKDBG_PWRITEV_RMW_AFTER_TAIL] = "pwritev_rmw.after_tail", > - [BLKDBG_PWRITEV] = "pwritev", > - [BLKDBG_PWRITEV_ZERO] = "pwritev_zero", > - [BLKDBG_PWRITEV_DONE] = "pwritev_done", > - > - [BLKDBG_EMPTY_IMAGE_PREPARE] = "empty_image_prepare", > -}; > - > -static int get_event_by_name(const char *name, BlkDebugEvent *event) > +static int get_event_by_name(const char *name, BlkdebugEvent *event) > { > int i; > > - for (i = 0; i < BLKDBG_EVENT_MAX; i++) { > - if (!strcmp(event_names[i], name)) { > + for (i = 0; i < BLKDBG_MAX; i++) { > + if (!strcmp(BlkdebugEvent_lookup[i], name)) { > *event = i; > return 0; > } > @@ -223,7 +166,7 @@ static int add_rule(void *opaque, QemuOpts *opts, Error **errp) > struct add_rule_data *d = opaque; > BDRVBlkdebugState *s = d->s; > const char* event_name; > - BlkDebugEvent event; > + BlkdebugEvent event; > struct BlkdebugRule *rule; > > /* Find the right event for the rule */ > @@ -563,7 +506,7 @@ static void blkdebug_close(BlockDriverState *bs) > BlkdebugRule *rule, *next; > int i; > > - for (i = 0; i < BLKDBG_EVENT_MAX; i++) { > + for (i = 0; i < BLKDBG_MAX; i++) { > QLIST_FOREACH_SAFE(rule, &s->rules[i], next, next) { > remove_rule(rule); > } > @@ -622,13 +565,13 @@ static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule, > return injected; > } > > -static void blkdebug_debug_event(BlockDriverState *bs, BlkDebugEvent event) > +static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event) > { > BDRVBlkdebugState *s = bs->opaque; > struct BlkdebugRule *rule, *next; > bool injected; > > - assert((int)event >= 0 && event < BLKDBG_EVENT_MAX); > + assert((int)event >= 0 && event < BLKDBG_MAX); > > injected = false; > s->new_state = s->state; > @@ -643,7 +586,7 @@ static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event, > { > BDRVBlkdebugState *s = bs->opaque; > struct BlkdebugRule *rule; > - BlkDebugEvent blkdebug_event; > + BlkdebugEvent blkdebug_event; > > if (get_event_by_name(event, &blkdebug_event) < 0) { > return -ENOENT; > @@ -685,7 +628,7 @@ static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs, > BlkdebugRule *rule, *next; > int i, ret = -ENOENT; > > - for (i = 0; i < BLKDBG_EVENT_MAX; i++) { > + for (i = 0; i < BLKDBG_MAX; i++) { > QLIST_FOREACH_SAFE(rule, &s->rules[i], next, next) { > if (rule->action == ACTION_SUSPEND && > !strcmp(rule->options.suspend.tag, tag)) { > diff --git a/docs/blkdebug.txt b/docs/blkdebug.txt > index b67a36d..43d8e8f 100644 > --- a/docs/blkdebug.txt > +++ b/docs/blkdebug.txt > @@ -1,6 +1,6 @@ > Block I/O error injection using blkdebug > ---------------------------------------- > -Copyright (C) 2014 Red Hat Inc > +Copyright (C) 2014-2015 Red Hat Inc > > This work is licensed under the terms of the GNU GPL, version 2 or later. See > the COPYING file in the top-level directory. > @@ -92,8 +92,9 @@ The core events are: > > flush_to_disk - flush the host block device's disk cache > > -See block/blkdebug.c:event_names[] for the full list of events. You may need > -to grep block driver source code to understand the meaning of specific events. > +See qapi/block-core.json:BlkdebugEvent for the full list of events. > +You may need to grep block driver source code to understand the > +meaning of specific events. > > State transitions > ----------------- > diff --git a/include/block/block.h b/include/block/block.h > index 73edb1a..2a72969 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -520,66 +520,6 @@ void bdrv_op_block_all(BlockDriverState *bs, Error *reason); > void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason); > bool bdrv_op_blocker_is_empty(BlockDriverState *bs); > > -typedef enum { > - BLKDBG_L1_UPDATE, > - > - BLKDBG_L1_GROW_ALLOC_TABLE, > - BLKDBG_L1_GROW_WRITE_TABLE, > - BLKDBG_L1_GROW_ACTIVATE_TABLE, > - > - BLKDBG_L2_LOAD, > - BLKDBG_L2_UPDATE, > - BLKDBG_L2_UPDATE_COMPRESSED, > - BLKDBG_L2_ALLOC_COW_READ, > - BLKDBG_L2_ALLOC_WRITE, > - > - BLKDBG_READ_AIO, > - BLKDBG_READ_BACKING_AIO, > - BLKDBG_READ_COMPRESSED, > - > - BLKDBG_WRITE_AIO, > - BLKDBG_WRITE_COMPRESSED, > - > - BLKDBG_VMSTATE_LOAD, > - BLKDBG_VMSTATE_SAVE, > - > - BLKDBG_COW_READ, > - BLKDBG_COW_WRITE, > - > - BLKDBG_REFTABLE_LOAD, > - BLKDBG_REFTABLE_GROW, > - BLKDBG_REFTABLE_UPDATE, > - > - BLKDBG_REFBLOCK_LOAD, > - BLKDBG_REFBLOCK_UPDATE, > - BLKDBG_REFBLOCK_UPDATE_PART, > - BLKDBG_REFBLOCK_ALLOC, > - BLKDBG_REFBLOCK_ALLOC_HOOKUP, > - BLKDBG_REFBLOCK_ALLOC_WRITE, > - BLKDBG_REFBLOCK_ALLOC_WRITE_BLOCKS, > - BLKDBG_REFBLOCK_ALLOC_WRITE_TABLE, > - BLKDBG_REFBLOCK_ALLOC_SWITCH_TABLE, > - > - BLKDBG_CLUSTER_ALLOC, > - BLKDBG_CLUSTER_ALLOC_BYTES, > - BLKDBG_CLUSTER_FREE, > - > - BLKDBG_FLUSH_TO_OS, > - BLKDBG_FLUSH_TO_DISK, > - > - BLKDBG_PWRITEV_RMW_HEAD, > - BLKDBG_PWRITEV_RMW_AFTER_HEAD, > - BLKDBG_PWRITEV_RMW_TAIL, > - BLKDBG_PWRITEV_RMW_AFTER_TAIL, > - BLKDBG_PWRITEV, > - BLKDBG_PWRITEV_ZERO, > - BLKDBG_PWRITEV_DONE, > - > - BLKDBG_EMPTY_IMAGE_PREPARE, > - > - BLKDBG_EVENT_MAX, > -} BlkDebugEvent; > - > #define BLKDBG_EVENT(child, evt) \ > do { \ > if (child) { \ > @@ -587,7 +527,7 @@ typedef enum { > } \ > } while (0) > > -void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event); > +void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event); > > int bdrv_debug_breakpoint(BlockDriverState *bs, const char *event, > const char *tag); > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 4012e36..66e208d 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -244,7 +244,7 @@ struct BlockDriver { > int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts, > BlockDriverAmendStatusCB *status_cb); > > - void (*bdrv_debug_event)(BlockDriverState *bs, BlkDebugEvent event); > + void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent event); > > /* TODO Better pass a option string/QDict/QemuOpts to add any rule? */ > int (*bdrv_debug_breakpoint)(BlockDriverState *bs, const char *event, > diff --git a/qapi/block-core.json b/qapi/block-core.json > index a07b13f..603eb60 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1776,8 +1776,10 @@ > # @BlkdebugEvent > # > # Trigger events supported by blkdebug. > +# > +# Since: 2.0 > ## > -{ 'enum': 'BlkdebugEvent', > +{ 'enum': 'BlkdebugEvent', 'prefix': 'BLKDBG', > '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', I'm no friend of the 'prefix' feature. You could avoid it here by renaming BlkdebugEvent to Blkdbg. No additional churn, because you already replace hand-written BlkDebugEvent by generated BlkdebugEvent. Alternatively, you could reduce churn by renaming it to BlkDebugEvent. Matter of taste. Perhaps Kevin has a preference.