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)
-- 
2.21.3

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?

Regards,
BALATON Zoltan

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.

> 
> Regards,
> BALATON Zoltan

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?

-- 
Eduardo

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

---

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 <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()?

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

Eduardo Habkost <ehabkost@redhat.com> writes:

> 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 <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()?
>
> 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.

I agree.

> We could change object_get_canonical_path_component() to not
> require callers to call g_free(),

I'll look into that.  It would fix a memory leak I created because I
didn't expect object_get_canonical_path_component() to return a malloced
string.

>                                   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()).

[...]