Re: [PATCH 1/2] hw/core/qdev.c: add qdev_get_human_name()

[Philippe Mathieu-Daudé]

On 23/1/24 09:15, Manos Pitsidianakis wrote:
> On Tue, 23 Jan 2024 10:13, Philippe Mathieu-Daudé <philmd@linaro.org> 
> wrote:
> > Hi Manos,
> >
> > On 23/1/24 09:09, Manos Pitsidianakis wrote:
> > > Add a simple method to return some kind of human readable identifier for
> > > use in error messages.
> > >
> > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> > > ---
> > >   hw/core/qdev.c         | 10 ++++++++++
> > >   include/hw/qdev-core.h | 15 +++++++++++++++
> > >   2 files changed, 25 insertions(+)
> > >
> > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > index 43d863b0c5..499f191826 100644
> > > --- a/hw/core/qdev.c
> > > +++ b/hw/core/qdev.c
> > > @@ -879,6 +879,16 @@ Object *qdev_get_machine(void)
> > >       return dev;
> > >   }
> > > +char *qdev_get_human_name(DeviceState *dev)
> > > +{
> > > +    if (!dev) {
> > > +        return g_strdup("");
> > > +    }
> > > +
> > > +    return dev->id ?
> > > +           g_strdup(dev->id) : object_get_canonical_path(OBJECT(dev));
> > > +}
> > > +
> > >   static MachineInitPhase machine_phase;
> > >   bool phase_check(MachineInitPhase phase)
> > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > > index 151d968238..a8c742b4a3 100644
> > > --- a/include/hw/qdev-core.h
> > > +++ b/include/hw/qdev-core.h
> > > @@ -993,6 +993,21 @@ const char *qdev_fw_name(DeviceState *dev);
> > >   void qdev_assert_realized_properly(void);
> > >   Object *qdev_get_machine(void);
> > > +/**
> > > + * qdev_get_human_name() - Return a human-readable name for a device
> > > + * @dev: The device
> > > + *
> > > + * .. note::
> > > + *    This function is intended for user friendly error messages.
> > > + *
> > > + * Returns: A newly allocated string containing the device id if not 
> > > null,
> > > + * else the object canonical path if not null. If @dev is NULL, it 
> > > returns an
> > > + * allocated empty string.
> >
> > In which case do we want to call this with NULL?
>
> None I could think of, just future-proofing the NULL case.

I'd rather have a raw assert() as future-proof API, avoiding
dubious corner cases :)

> > > + *
> > > + * Use g_free() to free it.
> > > + */
> > > +char *qdev_get_human_name(DeviceState *dev);
> > > +
> > >   /* FIXME: make this a link<> */
> > >   bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error 
> > > **errp);