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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 12 Apr 2018 11:15:04 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 29/03/2018 05:21, Alexey Kardashevskiy wrote:
> > +    DeviceState *dev = (DeviceState *) object_dynamic_cast(obj, TYPE_DEVICE);
> > +    const char *id = object_property_print(obj, "id", true, NULL);  
> 
> I learnt now about commit e1ff3c67e8544f41f1bea76ba76385faee0d2bb7 and I
> find it a mistake.  The "id" is available through
> object_get_canonical_path_component.  Can you change the "id" assignment
> to use object_get_canonical_path_component instead?

object_get_canonical_path_component() should be used with care as it
assumes that object HAS parent. It will crash if parent is NULL
or hasn't been assigned yet.

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.


> > +    const char *name = object_property_print(obj, "name", true, NULL);  
> 
> Who defines a name property?
> 
> > +    mon_printf(f, " %s:{", label);
> > +    if (dev) {
> > +        mon_printf(f, "dev");
> > +        if (dev->id) {
> > +            mon_printf(f, " id=%s", dev->id);
> > +        }
> > +    } else {
> > +        mon_printf(f, "obj");  
> 
> > +    }
> > +    if (name) {
> > +        mon_printf(f, " name=%s ", name);
> > +    }
> > +    if (id) {
> > +        mon_printf(f, " id=%s ", id);
> > +    }
> > +    if (dev && (!name && !id && !dev->id)) {
> > +        mon_printf(f, " path=%s", dev->canonical_path);  
> 
> Why not print the path for the !dev case?  I think we can just have
> 
>     mon_prinf(f, " %s:{%s", label, dev ? "dev" : "obj");
>     if (dev ? dev->id : id) {
>         mon_printf(f, " id=%s", dev ? dev->id : id);
>     } else {
>         canonical_path = object_get_canonical_path(...)
>         mon_printf(f, " path=%s", canonical_path);
>         g_free(canonical_path);
>     }
> 
> Thanks,
> 
> Paolo