Re: [RFC PATCH 2/3] hw/i2c/smbus_eeprom: Add description based on child name

[Philippe Mathieu-Daudé]

+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 <armbru@redhat.com>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@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()?

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;
         }
     }

---