Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qom/object.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 1812f79224..e06c78f9a5 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -571,18 +571,18 @@ void object_initialize_childv(Object *parentobj, const char *propname,
         }
     }
 
+out:
     /*
-     * Since object_property_add_child added a reference to the child object,
-     * we can drop the reference added by object_initialize(), so the child
-     * property will own the only reference to the object.
+     * We want @obj's reference to be 1 on success, 0 on failure.
+     * On success, it's 2: one taken by object_initialize(), and one
+     * by object_property_add_child().
+     * On failure in object_initialize() or earlier, it's 1.
+     * On failure afterwards, it's also 1: object_unparent() releases
+     * the reference taken by object_property_add_child().
      */
     object_unref(obj);
 
-out:
-    if (local_err) {
-        error_propagate(errp, local_err);
-        object_unref(obj);
-    }
+    error_propagate(errp, local_err);
 }
 
 static inline bool object_property_is_child(ObjectProperty *prop)
-- 
2.21.1

On 4/28/20 11:34 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qom/object.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 1812f79224..e06c78f9a5 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -571,18 +571,18 @@ void object_initialize_childv(Object *parentobj, const char *propname,
>           }
>       }
>   
> +out:
>       /*
> -     * Since object_property_add_child added a reference to the child object,
> -     * we can drop the reference added by object_initialize(), so the child
> -     * property will own the only reference to the object.
> +     * We want @obj's reference to be 1 on success, 0 on failure.
> +     * On success, it's 2: one taken by object_initialize(), and one
> +     * by object_property_add_child().
> +     * On failure in object_initialize() or earlier, it's 1.
> +     * On failure afterwards, it's also 1: object_unparent() releases
> +     * the reference taken by object_property_add_child().

Useful comment.

>        */
>       object_unref(obj);
>   
> -out:
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        object_unref(obj);
> -    }
> +    error_propagate(errp, local_err);

But looking at just the code (even without comments), this is safe - 
pre-patch, the only way to get to the out: label without a 'goto' is on 
the success path, at which point we can deduce that object_unref() is 
called exactly once whether on the success path or on the local_err 
path, at which point moving the label up and making the object_unref() 
unconditional is the same semantics.  With that hoisted, error_propagate 
is the only thing remaining under an 'if (local_err)' guard, and it is 
safe to call that when there is no error.  No semantic change, at which 
point all that remains to review is the accuracy of the comment.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org