Eduardo Habkost <ehabk...@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 <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.
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()).
[...]
