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

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

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;

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

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

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