[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 09/16] qemu-option: qemu_opts_from_qdict(): use
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 09/16] qemu-option: qemu_opts_from_qdict(): use error_set() |
Date: |
Fri, 18 May 2012 11:51:03 -0300 |
On Fri, 18 May 2012 16:43:16 +0200
Laszlo Ersek <address@hidden> 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 <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.
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
>
- [Qemu-devel] [PATCH qmp-next v4 00/16]: qapi: convert netdev_add & netdev_del, Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 02/16] qemu-option: parse_option_number(): use error_set(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 01/16] qemu-option: qemu_opts_create(): use error_set(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 05/16] qemu-option: qemu_opt_parse(): use error_set(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 10/16] qerror: introduce QERR_INVALID_OPTION_GROUP, Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 09/16] qemu-option: qemu_opts_from_qdict(): use error_set(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 07/16] qemu-option: opt_set(): use error_set(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 03/16] qemu-option: parse_option_bool(): use error_set(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 08/16] qemu-option: introduce qemu_opt_set_err(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 04/16] qemu-option: parse_option_size(): use error_set(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 13/16] net: purge the monitor object from all init functions, Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 11/16] qemu-config: find_list(): use error_set(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 15/16] qapi: convert netdev_add, Luiz Capitulino, 2012/05/17