diff mbox

[v2,02/15] do_device_add(): look up "device" opts list with qemu_find_opts_err()

Message ID 1360096768-8863-3-git-send-email-lersek@redhat.com
State New
Headers show

Commit Message

Laszlo Ersek Feb. 5, 2013, 8:39 p.m. UTC 
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(-)

Comments

Eduardo Habkost Feb. 7, 2013, 5:01 p.m. UTC | #1 
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
> 
>
Laszlo Ersek Feb. 7, 2013, 5:07 p.m. UTC | #2 
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
Luiz Capitulino Feb. 7, 2013, 5:47 p.m. UTC | #3 
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).
Markus Armbruster March 20, 2013, 1:25 p.m. UTC | #4 
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 mbox

Patch

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;