Re: [Qemu-devel] [PATCH v12 19/36] blkdebug: Merge hand-rolled and qapi BlkdebugEvent enum

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> 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.