object_new(FOO) returns an object with ref_cnt == 1
and following
  object_property_set_bool(cpuobj, true, "realized", NULL)
set parent of cpuobj to '/machine/unattached' which makes
ref_cnt == 2.

Since machvirt_init() doesn't take ownership of cpuobj
returned by object_new() it should explicitly drop
reference to cpuobj when dangling pointer is about to
go out of scope like it's done pc_new_cpu() to avoid
object leak.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/arm/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1f216cf..83adf3f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1376,6 +1376,7 @@ static void machvirt_init(MachineState *machine)
         }
 
         object_property_set_bool(cpuobj, true, "realized", NULL);
+        object_unref(cpuobj);
     }
     fdt_add_timer_nodes(vms);
     fdt_add_cpu_nodes(vms);
-- 
2.7.4

On 16 February 2017 at 13:57, Igor Mammedov <imammedo@redhat.com> wrote:
> object_new(FOO) returns an object with ref_cnt == 1
> and following
>   object_property_set_bool(cpuobj, true, "realized", NULL)
> set parent of cpuobj to '/machine/unattached' which makes
> ref_cnt == 2.
>
> Since machvirt_init() doesn't take ownership of cpuobj
> returned by object_new() it should explicitly drop
> reference to cpuobj when dangling pointer is about to
> go out of scope like it's done pc_new_cpu() to avoid
> object leak.

I've always found the object reference semantics somewhat
confusing (why does realizing a device add a reference,
for instance?). Do we document them anywhere?

thanks
-- PMM

On Thu, 16 Feb 2017 14:18:05 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 16 February 2017 at 13:57, Igor Mammedov <imammedo@redhat.com> wrote:
> > object_new(FOO) returns an object with ref_cnt == 1
> > and following
> >   object_property_set_bool(cpuobj, true, "realized", NULL)
> > set parent of cpuobj to '/machine/unattached' which makes
> > ref_cnt == 2.
> >
> > Since machvirt_init() doesn't take ownership of cpuobj
> > returned by object_new() it should explicitly drop
> > reference to cpuobj when dangling pointer is about to
> > go out of scope like it's done pc_new_cpu() to avoid
> > object leak.  
> 
> I've always found the object reference semantics somewhat
> confusing (why does realizing a device add a reference,
> for instance?). Do we document them anywhere?
I'm not aware of a place where it's documented.

currently device_realize() sets parent thus increasing
ref counter only if device creator haven't set parent
explicitly.

> 
> thanks
> -- PMM
> 

On 16 February 2017 at 15:11, Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 16 Feb 2017 14:18:05 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> I've always found the object reference semantics somewhat
>> confusing (why does realizing a device add a reference,
>> for instance?). Do we document them anywhere?
> I'm not aware of a place where it's documented.
>
> currently device_realize() sets parent thus increasing
> ref counter only if device creator haven't set parent
> explicitly.

It doesn't seem to:

static void device_realize(DeviceState *dev, Error **errp)
{
    DeviceClass *dc = DEVICE_GET_CLASS(dev);

    if (dc->init) {
        int rc = dc->init(dev);
        if (rc < 0) {
            error_setg(errp, "Device initialization failed.");
            return;
        }
    }
}

...it just calls the device's init function if it has one.

It's also pretty confusing that qdev_try_create()
and qdev_create() return a pointer to an object
that has been put into a bus and had unref called
(so the caller doesn't need to manually unref),
but plain object_new() returns a pointer to an
object that hasn't been put into a bus, yet
realizing does put it into a bus but doesn't do the
corresponding unref.

I'd be a lot happier if we had clear documentation that
described how our object model, plugging things into buses,
etc handled reference counting and what the expected
"correct" code patterns are for using it.

That said, I guess this patch is correct so I'm applying
it to target-arm.next.

thanks
-- PMM

On Fri, 17 Feb 2017 13:32:15 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 16 February 2017 at 15:11, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Thu, 16 Feb 2017 14:18:05 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:  
> >> I've always found the object reference semantics somewhat
> >> confusing (why does realizing a device add a reference,
> >> for instance?). Do we document them anywhere?  
> > I'm not aware of a place where it's documented.
> >
> > currently device_realize() sets parent thus increasing
> > ref counter only if device creator haven't set parent
> > explicitly.  
> 
> It doesn't seem to:
> 
> static void device_realize(DeviceState *dev, Error **errp)
> {
>     DeviceClass *dc = DEVICE_GET_CLASS(dev);
> 
>     if (dc->init) {
>         int rc = dc->init(dev);
>         if (rc < 0) {
>             error_setg(errp, "Device initialization failed.");
>             return;
>         }
>     }
> }
static void device_set_realized(Object *obj, bool value, Error **errp)
{
...
    if (value && !dev->realized) {                                               
        if (!obj->parent) {                                                      
            gchar *name = g_strdup_printf("device[%d]", unattached_count++);     
                                                                                 
            object_property_add_child(container_get(qdev_get_machine(),          
                                                    "/unattached"),              
                                      name, obj, &error_abort);                  
            unattached_parent = true;                                            
            g_free(name);                                                        
        }

 
> ...it just calls the device's init function if it has one.
> 
> It's also pretty confusing that qdev_try_create()
> and qdev_create() return a pointer to an object
> that has been put into a bus and had unref called
> (so the caller doesn't need to manually unref),
qdev_try_create() when puts device on bus,
it creates QOM link property to device which increases refcnt
qdev_try_create() -> qdev_set_parent_bus() -> bus_add_child()

link is not really usable at that time as device doesn't have
parent (in QOM terms) and attempt to resolve it to path would
assert, so it does set link manually by hack
bus_add_child()
    kid->child = child;                                                          
    object_ref(OBJECT(kid->child)); 

and then as bus holds reference, device won't disappear 
until it's attached to bus, it unrefs original
(qdev_try_create owned) pointer and returns pointer
owned by qdev framework.

later device creator calls
 qdev_init_nofail() ->
   object_property_set_bool(true, "realized");

which sets QOM parent for device to "/machine/unattached"
if caller hasn't set it manually,

like
 qdev_device_add() ->
    qdev_set_id() ->
       object_property_add_child("/peripheral" | "/peripheral-anon")

or
 ioapic_init_gsi() ->
     qdev_create()
     object_property_add_child(...)
     qdev_init_nofail()

> but plain object_new() returns a pointer to an
> object that hasn't been put into a bus, yet
it's like malloc/new and used for all objects including
ones without realize which is Device concept.
So naturally caller hold/owns the first reference
and should take care of it.

> realizing does put it into a bus but doesn't do the
> corresponding unref.
it might add extra reference so successfully created device
won't disappear once original owner 'frees' pointer that
goes out of scope.


> I'd be a lot happier if we had clear documentation that
> described how our object model, plugging things into buses,
> etc handled reference counting and what the expected
> "correct" code patterns are for using it.
I see 2 APIs in use:
 1: legacy qdev_create() + qdev_init_nofail()
     for hardwired on board devices bus attached oriented
 2: object_new() (+ device.realize() in case if object is Device)
     used by device_add() used for both bus/bus-less device
     post machine_init time.
    

> That said, I guess this patch is correct so I'm applying
> it to target-arm.next.
> 
> thanks
> -- PMM