Re: [Qemu-devel] [PATCH qemu] RFC: memory/hmp: Print owners/parents in "info mtree"

From: Igor Mammedov <imammedo@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Alexey Kardashevskiy <aik@ozlabs.ru>,
	qemu-devel@nongnu.org,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	Markus Armbruster <armbru@redhat.com>,
	peter.maydell@linaro.org
Subject: Re: [Qemu-devel] [PATCH qemu] RFC: memory/hmp: Print owners/parents in "info mtree"
Date: Tue, 17 Apr 2018 14:18:03 +0200	[thread overview]
Message-ID: <20180417141803.2d588b97@redhat.com> (raw)
In-Reply-To: <13008b1f-4b59-c6e1-69d2-98f8280f6f34@redhat.com>

On Mon, 16 Apr 2018 17:29:23 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 16/04/2018 16:27, Igor Mammedov wrote:
> > On Mon, 16 Apr 2018 15:30:39 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >   
> >> On 16/04/2018 15:20, Igor Mammedov wrote:  
> >>> Generally object doesn't need to know its own name,
> >>> we use it only for debugging and nice error reporting so far.
> >>> I'd rather have 'id' property at Object level so we won't have
> >>> to fish out ID from parent /which we aren't supposed to do and
> >>> which doesn't work in some cases/ when it's needed within
> >>> object itself.    
> >>
> >> Having an 'id' at object level is also a mess, because that id is
> >> invalid after unparent.  
> > I'd just consider 'id' as object name which is valid even if there
> > is no parent (during whole object lifecycle).  
> 
> What's the point of an object name if it cannot be unique?
It should be sufficient for it to be unique within parent's
scope and object_property_add_child() should make sure that
added object is unique within its parent's namespace.
Having named object from starters is useful as object
won't have to piggyback on parent (object_get_canonical_path_component)
when it need its own name. Then named object could use its name
freely anywhere including initfn, property setters/getters and
let object_property_add_child() take care of possible name
conflict.
From debugging/error reporting pov 'id' might be ambiguous
sometimes but it's way better than unanimous "NULL" string
or crash due to no set parent.

It sort of relates to discussion Peter started with

https://patchwork.kernel.org/patch/10224775/
"
Code that used to look like this:
    object_initialize(&s->ic, sizeof(s->ic), TYPE_BCM2835_IC);
    object_property_add_child(obj, "ic", OBJECT(&s->ic), NULL);
    qdev_set_parent_bus(DEVICE(&s->ic), sysbus_get_default());
can now look like this:
    sysbus_init_child(obj, "ic", &s->ic, sizeof(s->ic), TYPE_BCM2835_IC);
" 

Considering that in most cases where named objects are used
we parent them. We might be better of with API like

named_object_new(TYPE, NAME, PARENT, ERROR)
named_object_initialize(PTR, SIZE, TYPE, NAME, PARENT, ERROR)

> Paolo
> 
> > That would allow for object to have a reachable name vs getting NULL
> > when parent isn't set.
> > Maybe Object::id is overkill, but we probably could use NamedObject
> > where it's needed and avoid reverse engineering id from path.
> >    
> >> Since this is just for debugging use,
> >> object_get_canonical_path_component is the right function.  We can just
> >> make it return NULL if there is no parent.  
> >
> > looking at current use it out-grew just debugging usecases
> > and it's rather messy right now:
> > 
> > ram_backend_memory_alloc, throttle_group_obj_complete,
> > xlnx_zynqmp_create_rpu, spapr_drc.c:realize, iothread_complete,
> > memory_region_name  
> 
> I agree, _but_ it's definitely okay for debugging usecases.
yep, since there isn't other way to do it now,
it's fine to use object_get_canonical_path_component()

> 
> Paolo