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

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

[Stefan Hajnoczi]

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 <stefanha@redhat.com>
---
 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;
+    }
 
     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

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 <stefanha@redhat.com>
> ---
>  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
> 

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

On Sat, Sep 20, 2014 at 09:32:35AM +0000, Gonglei (Arei) wrote:
> > @@ -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.

Setting up the rules has failed and we need to free the QemuOpts which
were built up in this function.

If we don't free them then there is a memory leak.

Why is it correct to avoid resetting set_state_opts?

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Gonglei (Arei)]

> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> Sent: Monday, September 22, 2014 6:28 PM
> Subject: Re: [Qemu-devel] [PATCH] blkdebug: show an error for invalid event
> names
> 
> On Sat, Sep 20, 2014 at 09:32:35AM +0000, Gonglei (Arei) wrote:
> > > @@ -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.
> 
> Setting up the rules has failed and we need to free the QemuOpts which
> were built up in this function.
> 
> If we don't free them then there is a memory leak.
> 
Yes. My fault. :( Sorry.

The QemuOpts created in qemu_config_parse(), So they need be freed
If encounter any errors after this calling...

Reviewed-by: Gonglei <arei.gonglei@huawei.com>

Best regards,
-Gonglei

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

[Kevin Wolf]

Am 20.09.2014 um 10:55 hat Stefan Hajnoczi geschrieben:
> 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 <stefanha@redhat.com>

Thanks, applied to the block branch.

Kevin