Re: [Qemu-devel] [PATCH 1/2] qdev_monitor: Simplify error handling in qdev_device_add()

[Igor Mammedov]

On Thu,  5 Oct 2017 14:32:17 +0200
Thomas Huth <address@hidden> wrote:

> Instead of doing the clean-ups on errors multiple times, introduce
> a jump label at the end of the function that can be used by all
> error paths that need this cleanup.
> 
> Suggested-by: Igor Mammedov <address@hidden>
> Signed-off-by: Thomas Huth <address@hidden>
> ---
>  qdev-monitor.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 8fd6df9..cb2b109 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -620,22 +620,21 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
> **errp)
>  
>      /* set properties */
>      if (qemu_opt_foreach(opts, set_property, dev, &err)) {
> -        error_propagate(errp, err);
> -        object_unparent(OBJECT(dev));
> -        object_unref(OBJECT(dev));
> -        return NULL;
> +        goto err_del_dev;
>      }
>  
>      dev->opts = opts;
>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
> -    if (err != NULL) {
> -        error_propagate(errp, err);
> -        dev->opts = NULL;
> -        object_unparent(OBJECT(dev));
> -        object_unref(OBJECT(dev));
> -        return NULL;
> +    if (!err) {
> +        return dev;
>      }
typically the same error check pattern is used through out the function
so I'd not do inversion here. i.e. keep normal flow non-branched and jump on 
error

 if (err != NULL) {
    goto err_del_dev;
 }
 return dev;

 err_del_dev:
    dev->opts = NULL;

> -    return dev;
> +    dev->opts = NULL;
> +
> +err_del_dev:
> +    error_propagate(errp, err);
> +    object_unparent(OBJECT(dev));
> +    object_unref(OBJECT(dev));
> +    return NULL;
>  }
>  
>