Re: [Qemu-devel] [PATCH 09/16] qemu-option: qemu_opts_from_qdict(): use error_set()

[Luiz Capitulino <lcapitulino@redhat.com>]

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
>