Re: [Qemu-devel] [PATCH] blkdebug: show an error for invalid event names

[Gonglei (Arei)]

> Subject: [Qemu-devel] [PATCH] blkdebug: show an error for invalid event names
> 
> It is easy to typo a blkdebug configuration and waste a lot of time
> figuring out why no rules are matching.
> 
> Push the Error** down into add_rule() so we can report an error when the
> event name is invalid.
> 
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  block/blkdebug.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 69b330e..988303a 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -217,6 +217,7 @@ static int get_event_by_name(const char *name,
> BlkDebugEvent *event)
>  struct add_rule_data {
>      BDRVBlkdebugState *s;
>      int action;
> +    Error **errp;
>  };
> 
>  static int add_rule(QemuOpts *opts, void *opaque)
> @@ -229,7 +230,11 @@ static int add_rule(QemuOpts *opts, void *opaque)
> 
>      /* Find the right event for the rule */
>      event_name = qemu_opt_get(opts, "event");
> -    if (!event_name || get_event_by_name(event_name, &event) < 0) {
> +    if (!event_name) {
> +        error_setg(d->errp, "Missing event name for rule");
> +        return -1;
> +    } else if (get_event_by_name(event_name, &event) < 0) {
> +        error_setg(d->errp, "Invalid event name \"%s\"", event_name);
>          return -1;
>      }
> 
> @@ -315,10 +320,21 @@ static int read_config(BDRVBlkdebugState *s, const
> char *filename,
> 
>      d.s = s;
>      d.action = ACTION_INJECT_ERROR;
> -    qemu_opts_foreach(&inject_error_opts, add_rule, &d, 0);
> +    d.errp = &local_err;
> +    qemu_opts_foreach(&inject_error_opts, add_rule, &d, 1);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> 

If this check failed, it don't need to reset &set_state_opts.
So, maybe you should goto appropriate error label as local_err
is detected as each relevant point.

Best regards,
-Gonglei

>      d.action = ACTION_SET_STATE;
> -    qemu_opts_foreach(&set_state_opts, add_rule, &d, 0);
> +    qemu_opts_foreach(&set_state_opts, add_rule, &d, 1);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> 
>      ret = 0;
>  fail:
> --
> 1.9.3
>