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

[Eduardo Habkost <ehabkost@redhat.com>]

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