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

2014-09-22 Thread Stefan Hajnoczi 
On Sat, Sep 20, 2014 at 09:32:35AM +, 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


pgpJY_XGrFIuR.pgp
Description: PGP signature

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

2014-09-22 Thread Gonglei (Arei) 
 From: Stefan Hajnoczi [mailto:stefa...@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 +, 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.gong...@huawei.com

Best regards,
-Gonglei

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

2014-09-22 Thread 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 stefa...@redhat.com

Thanks, applied to the block branch.

Kevin

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

2014-09-20 Thread 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 stefa...@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

2014-09-20 Thread 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 stefa...@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