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 <lcapitul...@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
