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

[Luiz Capitulino]

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 */
}

> +        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;