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

[Eduardo Habkost]

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