On Fri, Jul 10, 2020 at 11:05:29AM +0200, Philippe Mathieu-Daudé wrote: > +Stefan for tracing PoV > > On 7/9/20 9:48 PM, Eduardo Habkost wrote: > > On Fri, Jun 26, 2020 at 04:26:33PM +0200, Philippe Mathieu-Daudé wrote: > >> On 6/26/20 1:00 PM, BALATON Zoltan wrote: > >>> On Fri, 26 Jun 2020, Philippe Mathieu-Daudé wrote: > >>>> Suggested-by: Markus Armbruster <arm...@redhat.com> > >>>> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org> > >>>> --- > >>>> hw/i2c/smbus_eeprom.c | 3 +++ > >>>> 1 file changed, 3 insertions(+) > >>>> > >>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c > >>>> index 879fd7c416..22ba7b20d4 100644 > >>>> --- a/hw/i2c/smbus_eeprom.c > >>>> +++ b/hw/i2c/smbus_eeprom.c > >>>> @@ -47,6 +47,7 @@ typedef struct SMBusEEPROMDevice { > >>>> àààuint8_t *init_data; > >>>> àààuint8_t offset; > >>>> àààbool accessed; > >>>> +àààchar *description; > >>>> } SMBusEEPROMDevice; > >>>> > >>>> static uint8_t eeprom_receive_byte(SMBusDevice *dev) > >>>> @@ -134,7 +135,9 @@ static void smbus_eeprom_realize(DeviceState *dev, > >>>> Error **errp) > >>>> àààsmbus_eeprom_reset(dev); > >>>> àààif (eeprom->init_data == NULL) { > >>>> àààààààerror_setg(errp, "init_data cannot be > >>>> NULL"); > >>>> +àààààààreturn; > >>>> ààà} > >>>> +àààeeprom->description = > >>>> object_get_canonical_path_component(OBJECT(dev)); > >>>> } > >>>> > >>>> static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > >>> > >>> What is this for? Shouldn't this description field be in some parent > >>> object and whatever wants to print it could use the > >>> object_get_canonical_path_component() as default value if it's not set > >>> instead of adding more boiler plate to every object? > >> > >> You are right, if we want to use this field generically, it should be > >> a static Object field. I'll defer that question to Eduardo/Markus. > > > > I don't understand what's the question here. What would be the > > purpose of a static Object field, and why it would be better than > > simply calling object_get_canonical_path_component() when you > > need it? > > Because when reading a 8KB EEPROM with tracing enabled we end > up calling: > > 8192 g_hash_table_iter_init() > 8192 g_hash_table_iter_next() > 8192 object_property_iter_init() > 8192 object_property_iter_next() > 8192 g_hash_table_add() > 8192 g_strdup() > 8192 g_free() > > Which is why I added the SMBusEEPROMDeviceState::description > field, to call it once and cache it. But Zoltan realized this > could benefit all QOM objects, not this single one. > > So the question is, is it OK to make this a cached field > in object_get_canonical_path_component()?
That's what I was thinking of, but now I see that object_get_canonical_path_component() is an inconvenient API because it requires callers to g_free() the return value. We could change object_get_canonical_path_component() to not require callers to call g_free(), or create a new mechanism to get the object name like you suggested (and then get rid of most of the existing uses of object_get_canonical_path_component()). > > Something like (incomplete): > > -- >8 -- > --- a/qom/object.c > +++ b/qom/object.c > @@ -642,6 +642,7 @@ static void object_property_del_child(Object *obj, > Object *child) > break; > } > } > + g_free(child->canonical_path_component); > } > > void object_unparent(Object *obj) > @@ -1666,6 +1667,7 @@ object_property_add_child(Object *obj, const char > *name, > op->resolve = object_resolve_child_property; > object_ref(child); > child->parent = obj; > + child->canonical_path_component = > object_get_canonical_path_component(child); > return op; > } > > @@ -1901,6 +1903,10 @@ char *object_get_canonical_path_component(const > Object *obj) > return NULL; > } > > + if (obj->canonical_path_component) { > + return obj->canonical_path_component; > + } > + > g_hash_table_iter_init(&iter, obj->parent->properties); > while (g_hash_table_iter_next(&iter, NULL, (gpointer *)&prop)) { > if (!object_property_is_child(prop)) { > @@ -1908,7 +1914,8 @@ char *object_get_canonical_path_component(const > Object *obj) > } > > if (prop->opaque == obj) { > - return g_strdup(prop->name); > + obj->canonical_path_component_cached = g_strdup(prop->name); > + return obj->canonical_path_component_cached; > } > } > > --- > -- Eduardo