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

login
register
mail settings
Submitter Laszlo Ersek
Date Feb. 1, 2013, 5:38 p.m.
Message ID <1359740299-27968-3-git-send-email-lersek@redhat.com>
Download mbox | patch
Permalink /patch/217542/
State New
Headers show

Comments

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(-)
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

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;