[2/7] do_device_add(): look up "device" opts list with qemu_find_opts_err()

Submitted by Laszlo Ersek on Feb. 1, 2013, 5:38 p.m.

Details

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

Commit Message

Laszlo Ersek Feb. 1, 2013, 5:38 p.m.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/qdev-monitor.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

Comments

Luiz Capitulino Feb. 4, 2013, 4:38 p.m.
On Fri,  1 Feb 2013 18:38:14 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/qdev-monitor.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index 3ec9e49..32be5a2 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -590,14 +590,17 @@ 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;
>  
> -    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
> -    if (error_is_set(&local_err)) {
> +    if ((list = qemu_find_opts_err("device", &local_err)) == NULL ||
> +        (opts = qemu_opts_from_qdict(list, qdict, &local_err)) == NULL) {

I'm not against this change, but qemu_find_opts() should never fails, so
I don't mind the current code either.

Now, if you do change it, I'd recommend separating the checks above and
using a more standard idiom for checking for errors:

list = qemu_find_opts_err("device", &local_err);
if (error_is_set(&local_err)) {
	/* handle error */
}

opts = qemu_opts_from_qdict(..., &local_err);
if (error_is_set(&local_err)) {
	/* handle error */
}

> +        assert(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;
Laszlo Ersek Feb. 5, 2013, 12:04 a.m.
On 02/04/13 17:38, Luiz Capitulino wrote:
> On Fri,  1 Feb 2013 18:38:14 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  hw/qdev-monitor.c |    7 +++++--
>>  1 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
>> index 3ec9e49..32be5a2 100644
>> --- a/hw/qdev-monitor.c
>> +++ b/hw/qdev-monitor.c
>> @@ -590,14 +590,17 @@ 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;
>>  
>> -    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
>> -    if (error_is_set(&local_err)) {
>> +    if ((list = qemu_find_opts_err("device", &local_err)) == NULL ||
>> +        (opts = qemu_opts_from_qdict(list, qdict, &local_err)) == NULL) {
> 
> I'm not against this change, but qemu_find_opts() should never fails, so
> I don't mind the current code either.
> 
> Now, if you do change it, I'd recommend separating the checks above and
> using a more standard idiom for checking for errors:
> 
> list = qemu_find_opts_err("device", &local_err);
> if (error_is_set(&local_err)) {
> 	/* handle error */
> }
> 
> opts = qemu_opts_from_qdict(..., &local_err);
> if (error_is_set(&local_err)) {
> 	/* handle error */
> }

/* handle error */ is exactly the same for both checks (print it, free
it, bail out); I wanted to avoid duplicating that block. I'll redo it
without the assignments in the controlling expression but will keep the
handler block common if you don't mind.

Thanks!
Laszlo
Paolo Bonzini Feb. 5, 2013, 8:45 a.m.
Il 05/02/2013 01:04, Laszlo Ersek ha scritto:
>> > list = qemu_find_opts_err("device", &local_err);
>> > if (error_is_set(&local_err)) {
>> > 	/* handle error */
>> > }
>> > 
>> > opts = qemu_opts_from_qdict(..., &local_err);
>> > if (error_is_set(&local_err)) {
>> > 	/* handle error */
>> > }
> /* handle error */ is exactly the same for both checks (print it, free
> it, bail out); I wanted to avoid duplicating that block. I'll redo it
> without the assignments in the controlling expression but will keep the
> handler block common if you don't mind.

Would a goto help?

Paolo
Laszlo Ersek Feb. 5, 2013, 5:30 p.m.
On 02/05/13 09:45, Paolo Bonzini wrote:
> Il 05/02/2013 01:04, Laszlo Ersek ha scritto:
>>>> list = qemu_find_opts_err("device", &local_err);
>>>> if (error_is_set(&local_err)) {
>>>> 	/* handle error */
>>>> }
>>>>
>>>> opts = qemu_opts_from_qdict(..., &local_err);
>>>> if (error_is_set(&local_err)) {
>>>> 	/* handle error */
>>>> }
>> /* handle error */ is exactly the same for both checks (print it, free
>> it, bail out); I wanted to avoid duplicating that block. I'll redo it
>> without the assignments in the controlling expression but will keep the
>> handler block common if you don't mind.
> 
> Would a goto help?

In the end I'd like to restructure do_device_add() more deeply.
Extracting the HMP parts should simplify the control flow, it would work
toward said end automatically. For this series I wanted to keep the
impact contained. I haven't been on qemu-devel for a long time, but I've
already learned to (try to) minimize the attack surface.

Thanks
Laszlo

Patch hide | download patch | download mbox

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index 3ec9e49..32be5a2 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -590,14 +590,17 @@  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;
 
-    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
-    if (error_is_set(&local_err)) {
+    if ((list = qemu_find_opts_err("device", &local_err)) == NULL ||
+        (opts = qemu_opts_from_qdict(list, qdict, &local_err)) == NULL) {
+        assert(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;