Message ID | 1360096768-8863-3-git-send-email-lersek@redhat.com |
---|---|
State | New |
Headers | show |
On Tue, Feb 05, 2013 at 09:39:15PM +0100, Laszlo Ersek wrote: > Conversion status (call chains covered or substituted by error propagation > marked with square brackets): > > do_device_add -> [qemu_find_opts -> error_report] > do_device_add -> qdev_device_add -> qerror_report > do_device_add -> qdev_device_add -> qbus_find -> qbus_find_recursive > -> qerror_report > do_device_add -> qdev_device_add -> qbus_find -> qerror_report > do_device_add -> qdev_device_add -> set_property -> qdev_prop_parse > -> qerror_report_err > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > hw/qdev-monitor.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c > index 56d66c3..bbdc90f 100644 > --- a/hw/qdev-monitor.c > +++ b/hw/qdev-monitor.c > @@ -590,15 +590,20 @@ void do_info_qdm(Monitor *mon, const QDict *qdict) > int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data) > { > Error *local_err = NULL; > + QemuOptsList *list; > QemuOpts *opts; > DeviceState *dev; > > - opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err); > + list = qemu_find_opts_err("device", &local_err); > + if (!error_is_set(&local_err)) { > + opts = qemu_opts_from_qdict(list, qdict, &local_err); > + } Is this really worth the extra code complexity, if the "device" QemuOptsList is supposed to be always registered by QEMU? I would be happy enough with a simple "assert(list)" inside qemu_opts_from_qdict(). > 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)) { > qemu_opts_del(opts); > return 0; > -- > 1.7.1 > >
On 02/07/13 18:01, Eduardo Habkost wrote: > On Tue, Feb 05, 2013 at 09:39:15PM +0100, Laszlo Ersek wrote: >> Conversion status (call chains covered or substituted by error propagation >> marked with square brackets): >> >> do_device_add -> [qemu_find_opts -> error_report] >> do_device_add -> qdev_device_add -> qerror_report >> do_device_add -> qdev_device_add -> qbus_find -> qbus_find_recursive >> -> qerror_report >> do_device_add -> qdev_device_add -> qbus_find -> qerror_report >> do_device_add -> qdev_device_add -> set_property -> qdev_prop_parse >> -> qerror_report_err >> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> hw/qdev-monitor.c | 7 ++++++- >> 1 files changed, 6 insertions(+), 1 deletions(-) >> >> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c >> index 56d66c3..bbdc90f 100644 >> --- a/hw/qdev-monitor.c >> +++ b/hw/qdev-monitor.c >> @@ -590,15 +590,20 @@ void do_info_qdm(Monitor *mon, const QDict *qdict) >> int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data) >> { >> Error *local_err = NULL; >> + QemuOptsList *list; >> QemuOpts *opts; >> DeviceState *dev; >> >> - opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err); >> + list = qemu_find_opts_err("device", &local_err); >> + if (!error_is_set(&local_err)) { >> + opts = qemu_opts_from_qdict(list, qdict, &local_err); >> + } > > Is this really worth the extra code complexity, if the "device" > QemuOptsList is supposed to be always registered by QEMU? I would be > happy enough with a simple "assert(list)" inside qemu_opts_from_qdict(). I don't know. I was told to propagate errors and that's what I attempted to do. When in doubt, I try to go for consistency. I'd be fine either way (especially because this patch can be easily dropped). Thank you for reviewing the series! Laszlo
On Thu, 07 Feb 2013 18:07:01 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > On 02/07/13 18:01, Eduardo Habkost wrote: > > On Tue, Feb 05, 2013 at 09:39:15PM +0100, Laszlo Ersek wrote: > >> Conversion status (call chains covered or substituted by error propagation > >> marked with square brackets): > >> > >> do_device_add -> [qemu_find_opts -> error_report] > >> do_device_add -> qdev_device_add -> qerror_report > >> do_device_add -> qdev_device_add -> qbus_find -> qbus_find_recursive > >> -> qerror_report > >> do_device_add -> qdev_device_add -> qbus_find -> qerror_report > >> do_device_add -> qdev_device_add -> set_property -> qdev_prop_parse > >> -> qerror_report_err > >> > >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > >> --- > >> hw/qdev-monitor.c | 7 ++++++- > >> 1 files changed, 6 insertions(+), 1 deletions(-) > >> > >> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c > >> index 56d66c3..bbdc90f 100644 > >> --- a/hw/qdev-monitor.c > >> +++ b/hw/qdev-monitor.c > >> @@ -590,15 +590,20 @@ void do_info_qdm(Monitor *mon, const QDict *qdict) > >> int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data) > >> { > >> Error *local_err = NULL; > >> + QemuOptsList *list; > >> QemuOpts *opts; > >> DeviceState *dev; > >> > >> - opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err); > >> + list = qemu_find_opts_err("device", &local_err); > >> + if (!error_is_set(&local_err)) { > >> + opts = qemu_opts_from_qdict(list, qdict, &local_err); > >> + } > > > > Is this really worth the extra code complexity, if the "device" > > QemuOptsList is supposed to be always registered by QEMU? I would be > > happy enough with a simple "assert(list)" inside qemu_opts_from_qdict(). > > I don't know. I was told to propagate errors and that's what I attempted > to do. When in doubt, I try to go for consistency. I'd be fine either > way (especially because this patch can be easily dropped). Either way is fine (iirc I made the same comment against v1).
Laszlo Ersek <lersek@redhat.com> writes: > Conversion status (call chains covered or substituted by error propagation > marked with square brackets): > > do_device_add -> [qemu_find_opts -> error_report] > do_device_add -> qdev_device_add -> qerror_report > do_device_add -> qdev_device_add -> qbus_find -> qbus_find_recursive > -> qerror_report > do_device_add -> qdev_device_add -> qbus_find -> qerror_report > do_device_add -> qdev_device_add -> set_property -> qdev_prop_parse > -> qerror_report_err > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > hw/qdev-monitor.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c > index 56d66c3..bbdc90f 100644 > --- a/hw/qdev-monitor.c > +++ b/hw/qdev-monitor.c > @@ -590,15 +590,20 @@ void do_info_qdm(Monitor *mon, const QDict *qdict) > int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data) > { > Error *local_err = NULL; > + QemuOptsList *list; > QemuOpts *opts; > DeviceState *dev; > > - opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err); > + list = qemu_find_opts_err("device", &local_err); > + if (!error_is_set(&local_err)) { > + opts = qemu_opts_from_qdict(list, 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)) { > qemu_opts_del(opts); > return 0; } dev = qdev_device_add(opts, &local_err); /work/armbru/qemu/qdev-monitor.c:667:9: warning: ‘opts’ may be used uninitialized in this function [-Wmaybe-uninitialized] Not really, of course. We get here only if local_err is clear, and then we surely set opts. Recommend "fix" this warning by dropping this patch. qemu_find_opts("device") can't fail. We already assume that elsewhere, e.g. in drive_init(), balloon_parse(), virtcon_parse(), ...
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c index 56d66c3..bbdc90f 100644 --- a/hw/qdev-monitor.c +++ b/hw/qdev-monitor.c @@ -590,15 +590,20 @@ void do_info_qdm(Monitor *mon, const QDict *qdict) int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data) { Error *local_err = NULL; + QemuOptsList *list; QemuOpts *opts; DeviceState *dev; - opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err); + list = qemu_find_opts_err("device", &local_err); + if (!error_is_set(&local_err)) { + opts = qemu_opts_from_qdict(list, 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)) { qemu_opts_del(opts); return 0;
Conversion status (call chains covered or substituted by error propagation marked with square brackets): do_device_add -> [qemu_find_opts -> error_report] do_device_add -> qdev_device_add -> qerror_report do_device_add -> qdev_device_add -> qbus_find -> qbus_find_recursive -> qerror_report do_device_add -> qdev_device_add -> qbus_find -> qerror_report do_device_add -> qdev_device_add -> set_property -> qdev_prop_parse -> qerror_report_err Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- hw/qdev-monitor.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)