I had to inspect the code to learn whether a final virObjectUnref()
calls ALL dispose callbacks in child-to-parent order (akin to C++
destructors), or whether I manually had to call a parent-class dispose
when writing a child class dispose method.  The answer is the
former. (Thankfully, since VIR_FREE wipes out pointers for safety,
even if I had guessed wrong, I probably would not have tripped over a
double-free fault when the parent dispose ran for the second time).
While at it, the VIR_CLASS_NEW macro requires that the virObject
component at offset 0 be reached through the name 'parent', not
'object'.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/util/virobject.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/util/virobject.c b/src/util/virobject.c
index f08c18ce44..462b3d7d3f 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -147,10 +147,11 @@ virClassForObjectRWLockable(void)
  *
  * Register a new object class with @name. The @objectSize
  * should give the total size of the object struct, which
- * is expected to have a 'virObject object;' field as its
- * first member. When the last reference on the object is
- * released, the @dispose callback will be invoked to free
- * memory of the object fields
+ * is expected to have a 'virObject parent;' field as (or
+ * contained in) its first member. When the last reference
+ * on the object is released, the @dispose callback will be
+ * invoked to free memory of the local object fields, as
+ * well as dispose callbacks of the parent classes
  *
  * Returns a new class instance
  */
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Mar 14, 2019 at 11:24:47PM -0500, Eric Blake wrote:
> I had to inspect the code to learn whether a final virObjectUnref()
> calls ALL dispose callbacks in child-to-parent order (akin to C++
> destructors), or whether I manually had to call a parent-class dispose
> when writing a child class dispose method.  The answer is the
> former. (Thankfully, since VIR_FREE wipes out pointers for safety,
> even if I had guessed wrong, I probably would not have tripped over a
> double-free fault when the parent dispose ran for the second time).
> While at it, the VIR_CLASS_NEW macro requires that the virObject
> component at offset 0 be reached through the name 'parent', not
> 'object'.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  src/util/virobject.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index f08c18ce44..462b3d7d3f 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -147,10 +147,11 @@ virClassForObjectRWLockable(void)
>   *
>   * Register a new object class with @name. The @objectSize
>   * should give the total size of the object struct, which
> - * is expected to have a 'virObject object;' field as its
> - * first member. When the last reference on the object is
> - * released, the @dispose callback will be invoked to free
> - * memory of the object fields
> + * is expected to have a 'virObject parent;' field as (or
> + * contained in) its first member. When the last reference
> + * on the object is released, the @dispose callback will be
> + * invoked to free memory of the local object fields, as
> + * well as dispose callbacks of the parent classes
>   *
>   * Returns a new class instance

I agree, but I also think that this would also be worth mentioning at
virObjectUnref(), it currently says it will run the dispose callback associated
with the object class, but one has to look at the code carefully to see it
"climbs" up the chain through all the parents and disposes of everything.

Reviewed-by: Erik Skultety <eskultet@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 3/15/19 2:40 AM, Erik Skultety wrote:
> On Thu, Mar 14, 2019 at 11:24:47PM -0500, Eric Blake wrote:
>> I had to inspect the code to learn whether a final virObjectUnref()
>> calls ALL dispose callbacks in child-to-parent order (akin to C++
>> destructors), or whether I manually had to call a parent-class dispose
>> when writing a child class dispose method.  The answer is the
>> former. (Thankfully, since VIR_FREE wipes out pointers for safety,
>> even if I had guessed wrong, I probably would not have tripped over a
>> double-free fault when the parent dispose ran for the second time).
>> While at it, the VIR_CLASS_NEW macro requires that the virObject
>> component at offset 0 be reached through the name 'parent', not
>> 'object'.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  src/util/virobject.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>

> I agree, but I also think that this would also be worth mentioning at
> virObjectUnref(), it currently says it will run the dispose callback associated
> with the object class, but one has to look at the code carefully to see it
> "climbs" up the chain through all the parents and disposes of everything.

Yep, will expand to cover that as well, and push. Thanks for the review.

> 
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
> 

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list