| Message ID | 20230216122524.67212-6-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | bulk: Have object_child_foreach() take Error* and return boolean | expand |
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. > + 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. Thanks, C. > > - return args.obj ? IPMI_BMC(args.obj) : NULL; > + return IPMI_BMC(obj); > }
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); >> } >
On 2/16/23 20:16, Philippe Mathieu-Daudé wrote: > 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. But it should be an error ! :) See the error_setg below. >>> + 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/ even when CONFIG_QOM_CAST_DEBUG is set ? I might have missed something. Thanks, C. > > 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); >>> } >> >
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > 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; > + if (object_dynamic_cast(child, TYPE_IPMI_BMC)) { > + if (*obj) { > + return true; Looks like you're changing return false to return true. Intendional? > } > - 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; > } > > - return args.obj ? IPMI_BMC(args.obj) : NULL; > + return IPMI_BMC(obj); > }
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; + 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; } - return args.obj ? IPMI_BMC(args.obj) : NULL; + return IPMI_BMC(obj); }
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(-)