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

[Laszlo Ersek]

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 <address@hidden>
> ---
>  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