Re: [Qemu-arm] [Qemu-devel] [PATCH] hw/arm/virt: fix cpu object reference leak

[Igor Mammedov]

On Fri, 17 Feb 2017 13:32:15 +0000
Peter Maydell <address@hidden> wrote:

> On 16 February 2017 at 15:11, Igor Mammedov <address@hidden> wrote:
> > On Thu, 16 Feb 2017 14:18:05 +0000
> > Peter Maydell <address@hidden> 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