Re: [RFC PATCH 5/5] hw/ppc/pnv_bmc: Simplify pnv_bmc_find()

[Philippe Mathieu-Daudé]

On 16/2/23 19:12, Cédric Le Goater wrote:
> On 2/16/23 13:25, Philippe Mathieu-Daudé wrote:
> > ForeachArgs::name is only used once as TYPE_IPMI_BMC.
> > Since the penultimate commit, object_child_foreach_recursive()'s
> > handler takes an Error* argument and return a boolean.
> > We can directly pass ForeachArgs::obj as context, removing the
> > ForeachArgs structure.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >
> > ---
> > RFC: please double-check...
> >
> >
> >   hw/ppc/pnv_bmc.c | 25 +++++++++----------------
> >   1 file changed, 9 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
> > index 05acc88a55..566284469f 100644
> > --- a/hw/ppc/pnv_bmc.c
> > +++ b/hw/ppc/pnv_bmc.c
> > @@ -278,36 +278,29 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
> >       return IPMI_BMC(obj);
> >   }
> > -typedef struct ForeachArgs {
> > -    const char *name;
> > -    Object *obj;
> > -} ForeachArgs;
> > -
> >   static bool bmc_find(Object *child, void *opaque, Error **errp)
> >   {
> > -    ForeachArgs *args = opaque;
> > +    Object **obj = opaque;
> > -    if (object_dynamic_cast(child, args->name)) {
> > -        if (args->obj) {
> > -            return false;
>
> The purpose of this test was to catch multiple bmc devices and it's removed
> now.

Great.

> > +    if (object_dynamic_cast(child, TYPE_IPMI_BMC)) {
> > +        if (*obj) {
> > +            return true;
> >           }
> > -        args->obj = child;
> > +        *obj = child;
> >       }
> >       return true;
> >   }
> >   IPMIBmc *pnv_bmc_find(Error **errp)
> >   {
> > -    ForeachArgs args = { TYPE_IPMI_BMC, NULL };
> > -    int ret;
> > +    Object *obj = NULL;
> > -    ret = object_child_foreach_recursive(object_get_root(), bmc_find,
> > -                                         &args, NULL);
> > -    if (ret) {
> > +    if (!object_child_foreach_recursive(object_get_root(), bmc_find, 
> > &obj,
> > +                                        NULL)) {
> >           error_setg(errp, "machine should have only one BMC device. "
> >                      "Use '-nodefaults'");>           return NULL;
> >       }
>
> We don't test obj against NULL any more. This could break the QOM cast 
> below.

IIUC QOM cast-macros are NULL-safe, see
https://lore.kernel.org/qemu-devel/20210107121304.1db97130@bahia.lan/

If you concur I'll try to update the QOM API doc where relevant.

> > -    return args.obj ? IPMI_BMC(args.obj) : NULL;
> > +    return IPMI_BMC(obj);
> >   }