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