diff mbox

[09/16] qemu-option: qemu_opts_from_qdict(): use error_set()

Message ID 1337265221-7136-10-git-send-email-lcapitulino@redhat.com
State New
Headers show

Commit Message

Luiz Capitulino May 17, 2012, 2:33 p.m. UTC 
do_device_add() and do_netdev_add() call qerror_report_err() to maintain
their QError semantics.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/qdev-monitor.c |    7 +++++--
 net.c             |    5 ++++-
 qemu-option.c     |   31 ++++++++++++++++++++++++-------
 qemu-option.h     |    3 ++-
 4 files changed, 35 insertions(+), 11 deletions(-)

Comments

Laszlo Ersek May 18, 2012, 2:43 p.m. UTC | #1 
comments in-line

On 05/17/12 16:33, Luiz Capitulino wrote:
> do_device_add() and do_netdev_add() call qerror_report_err() to maintain
> their QError semantics.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  hw/qdev-monitor.c |    7 +++++--
>  net.c             |    5 ++++-
>  qemu-option.c     |   31 ++++++++++++++++++++++++-------
>  qemu-option.h     |    3 ++-
>  4 files changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index eed781d..b01ef06 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -554,10 +554,13 @@ void do_info_qdm(Monitor *mon)
>  
>  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
> +    Error *local_err = NULL;
>      QemuOpts *opts;
>  
> -    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict);
> -    if (!opts) {
> +    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
> +    if (error_is_set(&local_err)) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
>          return -1;
>      }
>      if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
> diff --git a/net.c b/net.c
> index f5d9cc7..246209f 100644
> --- a/net.c
> +++ b/net.c
> @@ -1237,11 +1237,14 @@ void net_host_device_remove(Monitor *mon, const QDict *qdict)
>  
>  int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
> +    Error *local_err = NULL;
>      QemuOpts *opts;
>      int res;
>  
> -    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict);
> +    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &local_err);
>      if (!opts) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
>          return -1;
>      }

AFAICS the error condition is not the same in these two callers, but
looking at the new qemu_opts_from_qdict() below, they should be
equivalent. OK.


>  
> diff --git a/qemu-option.c b/qemu-option.c
> index afee3fb..a26c40a 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -971,13 +971,19 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
>      assert(opts);
>  }
>  
> +typedef struct OptsFromQDictState {
> +    QemuOpts *opts;
> +    Error **errp;
> +} OptsFromQDictState;
> +
>  static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
>  {
> +    OptsFromQDictState *state = opaque;
>      char buf[32];
>      const char *value;
>      int n;
>  
> -    if (!strcmp(key, "id")) {
> +    if (!strcmp(key, "id") || error_is_set(state->errp)) {
>          return;
>      }

(Could have reversed the order, but I'm splitting hairs.)


>  
> @@ -1005,7 +1011,8 @@ static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
>      default:
>          return;
>      }
> -    qemu_opt_set(opaque, key, value);
> +
> +    qemu_opt_set_err(state->opts, key, value, state->errp);
>  }

Aha. qemu_opt_set() did report errors, we just didn't care in
qemu_opts_from_qdict_1(), and certainly didn't try to pass those
outwards, for stopping the iteration (or at least making the rest of the
iteration a no-op) or otherwise. This changed now.


>  
>  /*
> @@ -1014,21 +1021,31 @@ static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
>   * Only QStrings, QInts, QFloats and QBools are copied.  Entries with
>   * other types are silently ignored.
>   */
> -QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict)
> +QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> +                               Error **errp)
>  {
> -    QemuOpts *opts;
> +    OptsFromQDictState state;
>      Error *local_err = NULL;
> +    QemuOpts *opts;
>  
>      opts = qemu_opts_create(list, qdict_get_try_str(qdict, "id"), 1,
>                              &local_err);
>      if (error_is_set(&local_err)) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> +        error_propagate(errp, local_err);
>          return NULL;
>      }
>  
>      assert(opts != NULL);
> -    qdict_iter(qdict, qemu_opts_from_qdict_1, opts);
> +
> +    state.errp = &local_err;
> +    state.opts = opts;
> +    qdict_iter(qdict, qemu_opts_from_qdict_1, &state);
> +    if (error_is_set(&local_err)) {
> +        error_propagate(errp, local_err);
> +        qemu_opts_del(opts);
> +        return NULL;
> +    }
> +
>      return opts;
>  }

Yes, this is new error handling here (and another possibility to return
NULL to our callers). Seems correct. Our callers had to handle NULL before.

>  
> diff --git a/qemu-option.h b/qemu-option.h
> index c0e022b..951dec3 100644
> --- a/qemu-option.h
> +++ b/qemu-option.h
> @@ -132,7 +132,8 @@ int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname
>  QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev);
>  void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
>                              int permit_abbrev);
> -QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict);
> +QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> +                               Error **errp);
>  QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
>  
>  typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);

Good.

Laszlo
Luiz Capitulino May 18, 2012, 2:51 p.m. UTC | #2 
On Fri, 18 May 2012 16:43:16 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> comments in-line
> 
> On 05/17/12 16:33, Luiz Capitulino wrote:
> > do_device_add() and do_netdev_add() call qerror_report_err() to maintain
> > their QError semantics.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  hw/qdev-monitor.c |    7 +++++--
> >  net.c             |    5 ++++-
> >  qemu-option.c     |   31 ++++++++++++++++++++++++-------
> >  qemu-option.h     |    3 ++-
> >  4 files changed, 35 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> > index eed781d..b01ef06 100644
> > --- a/hw/qdev-monitor.c
> > +++ b/hw/qdev-monitor.c
> > @@ -554,10 +554,13 @@ void do_info_qdm(Monitor *mon)
> >  
> >  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >  {
> > +    Error *local_err = NULL;
> >      QemuOpts *opts;
> >  
> > -    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict);
> > -    if (!opts) {
> > +    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
> > +    if (error_is_set(&local_err)) {
> > +        qerror_report_err(local_err);
> > +        error_free(local_err);
> >          return -1;
> >      }
> >      if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
> > diff --git a/net.c b/net.c
> > index f5d9cc7..246209f 100644
> > --- a/net.c
> > +++ b/net.c
> > @@ -1237,11 +1237,14 @@ void net_host_device_remove(Monitor *mon, const QDict *qdict)
> >  
> >  int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >  {
> > +    Error *local_err = NULL;
> >      QemuOpts *opts;
> >      int res;
> >  
> > -    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict);
> > +    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &local_err);
> >      if (!opts) {
> > +        qerror_report_err(local_err);
> > +        error_free(local_err);
> >          return -1;
> >      }
> 
> AFAICS the error condition is not the same in these two callers, but
> looking at the new qemu_opts_from_qdict() below, they should be
> equivalent. OK.
> 
> 
> >  
> > diff --git a/qemu-option.c b/qemu-option.c
> > index afee3fb..a26c40a 100644
> > --- a/qemu-option.c
> > +++ b/qemu-option.c
> > @@ -971,13 +971,19 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
> >      assert(opts);
> >  }
> >  
> > +typedef struct OptsFromQDictState {
> > +    QemuOpts *opts;
> > +    Error **errp;
> > +} OptsFromQDictState;
> > +
> >  static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
> >  {
> > +    OptsFromQDictState *state = opaque;
> >      char buf[32];
> >      const char *value;
> >      int n;
> >  
> > -    if (!strcmp(key, "id")) {
> > +    if (!strcmp(key, "id") || error_is_set(state->errp)) {
> >          return;
> >      }
> 
> (Could have reversed the order, but I'm splitting hairs.)
> 
> 
> >  
> > @@ -1005,7 +1011,8 @@ static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
> >      default:
> >          return;
> >      }
> > -    qemu_opt_set(opaque, key, value);
> > +
> > +    qemu_opt_set_err(state->opts, key, value, state->errp);
> >  }
> 
> Aha. qemu_opt_set() did report errors, we just didn't care in
> qemu_opts_from_qdict_1(), and certainly didn't try to pass those
> outwards, for stopping the iteration (or at least making the rest of the
> iteration a no-op) or otherwise. This changed now.

Yes, because we can't override state->errp. So we either, report the first or
the last error.

> 
> 
> >  
> >  /*
> > @@ -1014,21 +1021,31 @@ static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
> >   * Only QStrings, QInts, QFloats and QBools are copied.  Entries with
> >   * other types are silently ignored.
> >   */
> > -QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict)
> > +QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> > +                               Error **errp)
> >  {
> > -    QemuOpts *opts;
> > +    OptsFromQDictState state;
> >      Error *local_err = NULL;
> > +    QemuOpts *opts;
> >  
> >      opts = qemu_opts_create(list, qdict_get_try_str(qdict, "id"), 1,
> >                              &local_err);
> >      if (error_is_set(&local_err)) {
> > -        qerror_report_err(local_err);
> > -        error_free(local_err);
> > +        error_propagate(errp, local_err);
> >          return NULL;
> >      }
> >  
> >      assert(opts != NULL);
> > -    qdict_iter(qdict, qemu_opts_from_qdict_1, opts);
> > +
> > +    state.errp = &local_err;
> > +    state.opts = opts;
> > +    qdict_iter(qdict, qemu_opts_from_qdict_1, &state);
> > +    if (error_is_set(&local_err)) {
> > +        error_propagate(errp, local_err);
> > +        qemu_opts_del(opts);
> > +        return NULL;
> > +    }
> > +
> >      return opts;
> >  }
> 
> Yes, this is new error handling here (and another possibility to return
> NULL to our callers). Seems correct. Our callers had to handle NULL before.
> 
> >  
> > diff --git a/qemu-option.h b/qemu-option.h
> > index c0e022b..951dec3 100644
> > --- a/qemu-option.h
> > +++ b/qemu-option.h
> > @@ -132,7 +132,8 @@ int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname
> >  QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev);
> >  void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
> >                              int permit_abbrev);
> > -QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict);
> > +QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> > +                               Error **errp);
> >  QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
> >  
> >  typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);
> 
> Good.
> 
> Laszlo
>
diff mbox

Patch

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index eed781d..b01ef06 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -554,10 +554,13 @@  void do_info_qdm(Monitor *mon)
 
 int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
+    Error *local_err = NULL;
     QemuOpts *opts;
 
-    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict);
-    if (!opts) {
+    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return -1;
     }
     if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
diff --git a/net.c b/net.c
index f5d9cc7..246209f 100644
--- a/net.c
+++ b/net.c
@@ -1237,11 +1237,14 @@  void net_host_device_remove(Monitor *mon, const QDict *qdict)
 
 int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
+    Error *local_err = NULL;
     QemuOpts *opts;
     int res;
 
-    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict);
+    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &local_err);
     if (!opts) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return -1;
     }
 
diff --git a/qemu-option.c b/qemu-option.c
index afee3fb..a26c40a 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -971,13 +971,19 @@  void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
     assert(opts);
 }
 
+typedef struct OptsFromQDictState {
+    QemuOpts *opts;
+    Error **errp;
+} OptsFromQDictState;
+
 static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
 {
+    OptsFromQDictState *state = opaque;
     char buf[32];
     const char *value;
     int n;
 
-    if (!strcmp(key, "id")) {
+    if (!strcmp(key, "id") || error_is_set(state->errp)) {
         return;
     }
 
@@ -1005,7 +1011,8 @@  static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
     default:
         return;
     }
-    qemu_opt_set(opaque, key, value);
+
+    qemu_opt_set_err(state->opts, key, value, state->errp);
 }
 
 /*
@@ -1014,21 +1021,31 @@  static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
  * Only QStrings, QInts, QFloats and QBools are copied.  Entries with
  * other types are silently ignored.
  */
-QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict)
+QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
+                               Error **errp)
 {
-    QemuOpts *opts;
+    OptsFromQDictState state;
     Error *local_err = NULL;
+    QemuOpts *opts;
 
     opts = qemu_opts_create(list, qdict_get_try_str(qdict, "id"), 1,
                             &local_err);
     if (error_is_set(&local_err)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         return NULL;
     }
 
     assert(opts != NULL);
-    qdict_iter(qdict, qemu_opts_from_qdict_1, opts);
+
+    state.errp = &local_err;
+    state.opts = opts;
+    qdict_iter(qdict, qemu_opts_from_qdict_1, &state);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+        qemu_opts_del(opts);
+        return NULL;
+    }
+
     return opts;
 }
 
diff --git a/qemu-option.h b/qemu-option.h
index c0e022b..951dec3 100644
--- a/qemu-option.h
+++ b/qemu-option.h
@@ -132,7 +132,8 @@  int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname
 QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev);
 void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
                             int permit_abbrev);
-QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict);
+QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
+                               Error **errp);
 QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
 
 typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);