qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [RFC PATCH 2/3] hw/i2c/smbus_eeprom: Add description based on child name
Date: Mon, 13 Jul 2020 11:23:32 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux)

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

[...]

reply via email to

[Prev in Thread] Current Thread [Next in Thread]