Message ID | 1337265221-7136-10-git-send-email-lcapitulino@redhat.com |
---|---|
State | New |
Headers | show |
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
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 --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);
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(-)