Re: [Qemu-devel] [PATCH v2 10/54] block: use qemu_enum_parse() in blkdebug_debug_breakpoint

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> This slightly change the error report message from "Invalid event
> name" to "Invalid parameter".

Actually from

    qemu-system-x86_64: LOCATION: Invalid event name "VALUE"

to

    qemu-system-x86_64: LOCATION: invalid parameter value: VALUE

Let's get that exactly right in the commit message.

Slight degradation, but the message is sub-par even before the patch.
When complaining about a parameter value, both parameter name and value
should be mentioned, as the value may well not be unique.  Not this
patch's problem.

Aside: LOCATION points to where the blkdebug driver is configured.  It
should point to the error's actual location instead: the configuration
file named with config=...  Also not this patch's problem.

>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  block/blkdebug.c | 28 ++++++++--------------------
>  1 file changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index c19ab28f07..50edda2a31 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -32,6 +32,7 @@
>  #include "qapi/qmp/qbool.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qapi/util.h"
>  #include "sysemu/qtest.h"
>  
>  typedef struct BDRVBlkdebugState {
> @@ -149,20 +150,6 @@ static QemuOptsList *config_groups[] = {
>      NULL
>  };
>  
> -static int get_event_by_name(const char *name, BlkdebugEvent *event)
> -{
> -    int i;
> -
> -    for (i = 0; i < BLKDBG__MAX; i++) {
> -        if (!strcmp(BlkdebugEvent_lookup[i], name)) {
> -            *event = i;
> -            return 0;
> -        }
> -    }
> -
> -    return -1;
> -}
> -
>  struct add_rule_data {
>      BDRVBlkdebugState *s;
>      int action;
> @@ -173,7 +160,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;
> +    int event;
>      struct BlkdebugRule *rule;
>      int64_t sector;
>  
> @@ -182,8 +169,9 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
> **errp)
>      if (!event_name) {
>          error_setg(errp, "Missing event name for rule");
>          return -1;
> -    } else if (get_event_by_name(event_name, &event) < 0) {
> -        error_setg(errp, "Invalid event name \"%s\"", event_name);
> +    }
> +    event = qapi_enum_parse(BlkdebugEvent_lookup, event_name, BLKDBG__MAX, 
> -1, errp);

Long line.

> +    if (event < 0) {
>          return -1;
>      }
>  
> @@ -743,13 +731,13 @@ static int blkdebug_debug_breakpoint(BlockDriverState 
> *bs, const char *event,
>  {
>      BDRVBlkdebugState *s = bs->opaque;
>      struct BlkdebugRule *rule;
> -    BlkdebugEvent blkdebug_event;
> +    int blkdebug_event;
>  
> -    if (get_event_by_name(event, &blkdebug_event) < 0) {
> +    blkdebug_event = qapi_enum_parse(BlkdebugEvent_lookup, event, 
> BLKDBG__MAX, -1, NULL);

Long line.

> +    if (blkdebug_event < 0) {
>          return -ENOENT;
>      }
>  
> -
>      rule = g_malloc(sizeof(*rule));
>      *rule = (struct BlkdebugRule) {
>          .event  = blkdebug_event,

With the nits touched up:
Reviewed-by: Markus Armbruster <address@hidden>