[PATCH 0/2] Optimize user_creatable_add_type error path

[PATCH 0/2] Optimize user_creatable_add_type error path

[Zhenzhong Duan]

Hi,

This is a simple optimization to user_creatable_add_type error path.
Removed local_err and its check in err path, use *errp instead.

Thanks
Zhenzhong

Zhenzhong Duan (2):
  qom/object_interfaces: Remove unnecessary local error check
  qom/object_interfaces: Remove local_err in user_creatable_add_type

 qom/object_interfaces.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH 1/2] qom/object_interfaces: Remove unnecessary local_err check

[Zhenzhong Duan]

In the error return path, local_err is always set, no need to check it.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 qom/object_interfaces.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index e0833c8bfe..255a7bf659 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -128,13 +128,11 @@ Object *user_creatable_add_type(const char *type, const char *id,
         }
         goto out;
     }
-out:
-    if (local_err) {
-        error_propagate(errp, local_err);
-        object_unref(obj);
-        return NULL;
-    }
     return obj;
+out:
+    error_propagate(errp, local_err);
+    object_unref(obj);
+    return NULL;
 }
 
 void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
-- 
2.34.1

Re: [PATCH 1/2] qom/object_interfaces: Remove unnecessary local_err check

[Zhao Liu]

On Thu, Feb 29, 2024 at 11:37:38AM +0800, Zhenzhong Duan wrote:
> Date: Thu, 29 Feb 2024 11:37:38 +0800
> From: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Subject: [PATCH 1/2] qom/object_interfaces: Remove unnecessary local_err
>  check
> X-Mailer: git-send-email 2.34.1
> 
> In the error return path, local_err is always set, no need to check it.

The original error handling code indicates "local_err is always set",
and error_propagate() can handle the case that local_err is NULL.

> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  qom/object_interfaces.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index e0833c8bfe..255a7bf659 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -128,13 +128,11 @@ Object *user_creatable_add_type(const char *type, const char *id,
>          }
>          goto out;
>      }
> -out:
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        object_unref(obj);
> -        return NULL;
> -    }
>      return obj;
> +out:

Maybe rename this to "err:"? Since now it's just used to handle error,
and "goto err" seems more clear.

> +    error_propagate(errp, local_err);
> +    object_unref(obj);
> +    return NULL;
>  }
>  
>  void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
> -- 
> 2.34.1
> 

Otherwise,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

RE: [PATCH 1/2] qom/object_interfaces: Remove unnecessary local_err check

[Duan, Zhenzhong]

>-----Original Message-----
>From: Liu, Zhao1 <zhao1.liu@intel.com>
>Subject: Re: [PATCH 1/2] qom/object_interfaces: Remove unnecessary
>local_err check
>
>On Thu, Feb 29, 2024 at 11:37:38AM +0800, Zhenzhong Duan wrote:
>> Date: Thu, 29 Feb 2024 11:37:38 +0800
>> From: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> Subject: [PATCH 1/2] qom/object_interfaces: Remove unnecessary
>local_err
>>  check
>> X-Mailer: git-send-email 2.34.1
>>
>> In the error return path, local_err is always set, no need to check it.
>
>The original error handling code indicates "local_err is always set",
>and error_propagate() can handle the case that local_err is NULL.

Will do.

>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  qom/object_interfaces.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
>> index e0833c8bfe..255a7bf659 100644
>> --- a/qom/object_interfaces.c
>> +++ b/qom/object_interfaces.c
>> @@ -128,13 +128,11 @@ Object *user_creatable_add_type(const char
>*type, const char *id,
>>          }
>>          goto out;
>>      }
>> -out:
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> -        object_unref(obj);
>> -        return NULL;
>> -    }
>>      return obj;
>> +out:
>
>Maybe rename this to "err:"? Since now it's just used to handle error,
>and "goto err" seems more clear.

Good suggestion, will do.

Thanks
Zhenzhong

>
>> +    error_propagate(errp, local_err);
>> +    object_unref(obj);
>> +    return NULL;
>>  }
>>
>>  void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
>> --
>> 2.34.1
>>
>
>Otherwise,
>
>Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
>

[PATCH 2/2] qom/object_interfaces: Remove local_err in user_creatable_add_type

[Zhenzhong Duan]

In user_creatable_add_type, there is mixed usage of ERRP_GUARD and
local_err. This makes error_abort not taking effect in those callee
functions with local_err passed.

Now that we already has ERRP_GUARD, remove local_err and use *errp
instead.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 qom/object_interfaces.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 255a7bf659..165cd433e7 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -81,7 +81,6 @@ Object *user_creatable_add_type(const char *type, const char *id,
     ERRP_GUARD();
     Object *obj;
     ObjectClass *klass;
-    Error *local_err = NULL;
 
     if (id != NULL && !id_wellformed(id)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id", "an identifier");
@@ -109,20 +108,20 @@ Object *user_creatable_add_type(const char *type, const char *id,
 
     assert(qdict);
     obj = object_new(type);
-    object_set_properties_from_qdict(obj, qdict, v, &local_err);
-    if (local_err) {
+    object_set_properties_from_qdict(obj, qdict, v, errp);
+    if (*errp) {
         goto out;
     }
 
     if (id != NULL) {
         object_property_try_add_child(object_get_objects_root(),
-                                      id, obj, &local_err);
-        if (local_err) {
+                                      id, obj, errp);
+        if (*errp) {
             goto out;
         }
     }
 
-    if (!user_creatable_complete(USER_CREATABLE(obj), &local_err)) {
+    if (!user_creatable_complete(USER_CREATABLE(obj), errp)) {
         if (id != NULL) {
             object_property_del(object_get_objects_root(), id);
         }
@@ -130,7 +129,6 @@ Object *user_creatable_add_type(const char *type, const char *id,
     }
     return obj;
 out:
-    error_propagate(errp, local_err);
     object_unref(obj);
     return NULL;
 }
-- 
2.34.1

Re: [PATCH 2/2] qom/object_interfaces: Remove local_err in user_creatable_add_type

[Zhao Liu]

On Thu, Feb 29, 2024 at 11:37:39AM +0800, Zhenzhong Duan wrote:
> Date: Thu, 29 Feb 2024 11:37:39 +0800
> From: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Subject: [PATCH 2/2] qom/object_interfaces: Remove local_err in
>  user_creatable_add_type
> X-Mailer: git-send-email 2.34.1
> 
> In user_creatable_add_type, there is mixed usage of ERRP_GUARD and
> local_err. This makes error_abort not taking effect in those callee
> functions with local_err passed.
> 
> Now that we already has ERRP_GUARD, remove local_err and use *errp
> instead.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  qom/object_interfaces.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 255a7bf659..165cd433e7 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -81,7 +81,6 @@ Object *user_creatable_add_type(const char *type, const char *id,
>      ERRP_GUARD();
>      Object *obj;
>      ObjectClass *klass;
> -    Error *local_err = NULL;
>  
>      if (id != NULL && !id_wellformed(id)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id", "an identifier");
> @@ -109,20 +108,20 @@ Object *user_creatable_add_type(const char *type, const char *id,
>  
>      assert(qdict);
>      obj = object_new(type);
> -    object_set_properties_from_qdict(obj, qdict, v, &local_err);
> -    if (local_err) {
> +    object_set_properties_from_qdict(obj, qdict, v, errp);

It's better to make object_set_properties_from_qdict someting (e.g.,
boolean). Maybe an extra cleanup?

> +    if (*errp) {
>          goto out;
>      }
>  
>      if (id != NULL) {
>          object_property_try_add_child(object_get_objects_root(),
> -                                      id, obj, &local_err);
> -        if (local_err) {
> +                                      id, obj, errp);
> +        if (*errp) {
>              goto out;
>          }
>      }

Here we could check whether the returned ObjectProperty* is NULL instaed
of dereferencing errp.

Thanks,
Zhao

RE: [PATCH 2/2] qom/object_interfaces: Remove local_err in user_creatable_add_type

[Duan, Zhenzhong]

>-----Original Message-----
>From: Liu, Zhao1 <zhao1.liu@intel.com>
>Subject: Re: [PATCH 2/2] qom/object_interfaces: Remove local_err in
>user_creatable_add_type
>
>On Thu, Feb 29, 2024 at 11:37:39AM +0800, Zhenzhong Duan wrote:
>> Date: Thu, 29 Feb 2024 11:37:39 +0800
>> From: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> Subject: [PATCH 2/2] qom/object_interfaces: Remove local_err in
>>  user_creatable_add_type
>> X-Mailer: git-send-email 2.34.1
>>
>> In user_creatable_add_type, there is mixed usage of ERRP_GUARD and
>> local_err. This makes error_abort not taking effect in those callee
>> functions with local_err passed.
>>
>> Now that we already has ERRP_GUARD, remove local_err and use *errp
>> instead.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  qom/object_interfaces.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
>> index 255a7bf659..165cd433e7 100644
>> --- a/qom/object_interfaces.c
>> +++ b/qom/object_interfaces.c
>> @@ -81,7 +81,6 @@ Object *user_creatable_add_type(const char *type,
>const char *id,
>>      ERRP_GUARD();
>>      Object *obj;
>>      ObjectClass *klass;
>> -    Error *local_err = NULL;
>>
>>      if (id != NULL && !id_wellformed(id)) {
>>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id", "an
>identifier");
>> @@ -109,20 +108,20 @@ Object *user_creatable_add_type(const char
>*type, const char *id,
>>
>>      assert(qdict);
>>      obj = object_new(type);
>> -    object_set_properties_from_qdict(obj, qdict, v, &local_err);
>> -    if (local_err) {
>> +    object_set_properties_from_qdict(obj, qdict, v, errp);
>
>It's better to make object_set_properties_from_qdict someting (e.g.,
>boolean). Maybe an extra cleanup?

OK, will do.

>
>> +    if (*errp) {
>>          goto out;
>>      }
>>
>>      if (id != NULL) {
>>          object_property_try_add_child(object_get_objects_root(),
>> -                                      id, obj, &local_err);
>> -        if (local_err) {
>> +                                      id, obj, errp);
>> +        if (*errp) {
>>              goto out;
>>          }
>>      }
>
>Here we could check whether the returned ObjectProperty* is NULL instaed
>of dereferencing errp.

Indeed, that's better, will do.

Thanks
Zhenzhong