1. Patchew
  2. QEMU
  3. [PATCH 0/5] bulk: Have object_child_foreach() take Error* and return boolean
  4. View patch

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

Philippe Mathieu-Daudé posted 5 patches 2 years, 11 months ago
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <ani@anisinha.ca>, Jean-Philippe Brucker <jean-philippe@linaro.org>, Peter Maydell <peter.maydell@linaro.org>, Shannon Zhao <shannon.zhaosl@gmail.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Helge Deller <deller@gmx.de>, Peter Xu <peterx@redhat.com>, Jason Wang <jasowang@redhat.com>, Laurent Vivier <laurent@vivier.eu>, David Hildenbrand <david@redhat.com>, Xiao Guangrong <xiaoguangrong.eric@gmail.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, "Cédric Le Goater" <clg@kaod.org>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Greg Kurz <groug@kaod.org>, Yuval Shaia <yuval.shaia.ml@gmail.com>, Thomas Huth <thuth@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Fam Zheng <fam@euphon.net>, Alexander Bulekov <alxndr@bu.edu>, Bandan Das <bsd@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Darren Kenny <darren.kenny@oracle.com>, Qiuhao Li <Qiuhao.Li@outlook.com>
[RFC PATCH 5/5] hw/ppc/pnv_bmc: Simplify pnv_bmc_find()
Posted by Philippe Mathieu-Daudé 2 years, 11 months ago 
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;
         }
-        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);
 }
-- 
2.38.1
Re: [RFC PATCH 5/5] hw/ppc/pnv_bmc: Simplify pnv_bmc_find()
Posted by Markus Armbruster 2 years, 11 months ago 
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);
>  }
Re: [RFC PATCH 5/5] hw/ppc/pnv_bmc: Simplify pnv_bmc_find()
Posted by Cédric Le Goater 2 years, 11 months ago 
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);
>   }
Re: [RFC PATCH 5/5] hw/ppc/pnv_bmc: Simplify pnv_bmc_find()
Posted by Philippe Mathieu-Daudé 2 years, 11 months ago 
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);
>>   }
>
Re: [RFC PATCH 5/5] hw/ppc/pnv_bmc: Simplify pnv_bmc_find()
Posted by Cédric Le Goater 2 years, 11 months ago 
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);
>>>   }
>>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.