Re: [Qemu-devel] [PATCH 2/7] do_device_add(): look up "device" opts list with qemu_find_opts_err()

[Laszlo Ersek]

On 02/04/13 17:38, Luiz Capitulino wrote:
> On Fri,  1 Feb 2013 18:38:14 +0100
> Laszlo Ersek <address@hidden> wrote:
> 
>>
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>>  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