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