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 <imammedo@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 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;
     }
-    return dev;
+    dev->opts = NULL;
+
+err_del_dev:
+    error_propagate(errp, err);
+    object_unparent(OBJECT(dev));
+    object_unref(OBJECT(dev));
+    return NULL;
 }
 
 
-- 
1.8.3.1

On Thu,  5 Oct 2017 14:32:17 +0200
Thomas Huth <thuth@redhat.com> 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 <imammedo@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  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;
>  }
>  
>  

On Thu, Oct 05, 2017 at 03:59:12PM +0200, Igor Mammedov wrote:
> On Thu,  5 Oct 2017 14:32:17 +0200
> Thomas Huth <thuth@redhat.com> 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 <imammedo@redhat.com>
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  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;

I prefer this pattern also.  It makes the success/error paths
very easy to spot.

-- 
Eduardo