[PATCH v2 0/3] Simplify user_creatable_add_type error path

[PATCH v2 0/3] Simplify user_creatable_add_type error path

[Zhenzhong Duan]

Hi,

This is a simplification to user_creatable_add_type error path.
Removed local_err and its check in error path, check return value
instead.

Tested with make check and guest bootup.

Thanks
Zhenzhong

Changelog:
v2:
- Use err label to replace out label (Zhao Liu)
- Refine patch description (Zhao Liu)
- Make object_set_properties_from_qdict return bool (Zhao Liu)
- Check return value of object_property_try_add_child (Zhao Liu)
- Add R-B

Zhenzhong Duan (3):
  qom/object_interfaces: Remove unnecessary local error check
  qom/object_interfaces: Make object_set_properties_from_qdict return
    bool
  qom/object_interfaces: Remove local_err in user_creatable_add_type

 qom/object_interfaces.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] qom/object_interfaces: Remove unnecessary local error check

[Zhenzhong Duan]

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

Use err label instead of out label for error path.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 qom/object_interfaces.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index e0833c8bfe..70179877f1 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -111,14 +111,14 @@ Object *user_creatable_add_type(const char *type, const char *id,
     obj = object_new(type);
     object_set_properties_from_qdict(obj, qdict, v, &local_err);
     if (local_err) {
-        goto out;
+        goto err;
     }
 
     if (id != NULL) {
         object_property_try_add_child(object_get_objects_root(),
                                       id, obj, &local_err);
         if (local_err) {
-            goto out;
+            goto err;
         }
     }
 
@@ -126,15 +126,13 @@ Object *user_creatable_add_type(const char *type, const char *id,
         if (id != NULL) {
             object_property_del(object_get_objects_root(), id);
         }
-        goto out;
-    }
-out:
-    if (local_err) {
-        error_propagate(errp, local_err);
-        object_unref(obj);
-        return NULL;
+        goto err;
     }
     return obj;
+err:
+    error_propagate(errp, local_err);
+    object_unref(obj);
+    return NULL;
 }
 
 void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
-- 
2.34.1

[PATCH v2 2/3] qom/object_interfaces: Make object_set_properties_from_qdict return bool

[Zhenzhong Duan]

Make object_set_properties_from_qdict() return bool, so that
user_creatable_add_type() could check its return value instead
of local_err pointer.

Opportunistically, do the same change to check return value of
object_property_try_add_child() instead of local_err pointer.

Suggested-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 qom/object_interfaces.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 70179877f1..e17e2de46d 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -43,22 +43,25 @@ bool user_creatable_can_be_deleted(UserCreatable *uc)
     }
 }
 
-static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
+static bool object_set_properties_from_qdict(Object *obj, const QDict *qdict,
                                              Visitor *v, Error **errp)
 {
     const QDictEntry *e;
+    bool ret;
 
     if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
-        return;
+        return false;
     }
     for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-        if (!object_property_set(obj, e->key, v, errp)) {
+        ret = object_property_set(obj, e->key, v, errp);
+        if (!ret) {
             goto out;
         }
     }
-    visit_check_struct(v, errp);
+    ret = visit_check_struct(v, errp);
 out:
     visit_end_struct(v, NULL);
+    return ret;
 }
 
 void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
@@ -109,17 +112,13 @@ 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) {
+    if (!object_set_properties_from_qdict(obj, qdict, v, &local_err)) {
         goto err;
     }
 
-    if (id != NULL) {
-        object_property_try_add_child(object_get_objects_root(),
-                                      id, obj, &local_err);
-        if (local_err) {
+    if (id != NULL && !object_property_try_add_child(object_get_objects_root(),
+                                                     id, obj, &local_err)) {
             goto err;
-        }
     }
 
     if (!user_creatable_complete(USER_CREATABLE(obj), &local_err)) {
-- 
2.34.1

Re: [PATCH v2 2/3] qom/object_interfaces: Make object_set_properties_from_qdict return bool

[Zhao Liu]

On Mon, Mar 18, 2024 at 11:32:09AM +0800, Zhenzhong Duan wrote:
> Date: Mon, 18 Mar 2024 11:32:09 +0800
> From: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Subject: [PATCH v2 2/3] qom/object_interfaces: Make
>  object_set_properties_from_qdict return bool
> X-Mailer: git-send-email 2.34.1
> 
> Make object_set_properties_from_qdict() return bool, so that
> user_creatable_add_type() could check its return value instead
> of local_err pointer.
> 
> Opportunistically, do the same change to check return value of
> object_property_try_add_child() instead of local_err pointer.
> 
> Suggested-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  qom/object_interfaces.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)

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

[PATCH v2 3/3] 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 use ERRP_GUARD, remove local_err and pass errp.

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

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index e17e2de46d..2067bf2230 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -84,7 +84,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");
@@ -112,16 +111,16 @@ Object *user_creatable_add_type(const char *type, const char *id,
 
     assert(qdict);
     obj = object_new(type);
-    if (!object_set_properties_from_qdict(obj, qdict, v, &local_err)) {
+    if (!object_set_properties_from_qdict(obj, qdict, v, errp)) {
         goto err;
     }
 
     if (id != NULL && !object_property_try_add_child(object_get_objects_root(),
-                                                     id, obj, &local_err)) {
+                                                     id, obj, errp)) {
             goto err;
     }
 
-    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);
         }
@@ -129,7 +128,6 @@ Object *user_creatable_add_type(const char *type, const char *id,
     }
     return obj;
 err:
-    error_propagate(errp, local_err);
     object_unref(obj);
     return NULL;
 }
-- 
2.34.1

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

[Zhao Liu]

On Mon, Mar 18, 2024 at 11:32:10AM +0800, Zhenzhong Duan wrote:
> Date: Mon, 18 Mar 2024 11:32:10 +0800
> From: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Subject: [PATCH v2 3/3] 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 use ERRP_GUARD, remove local_err and pass errp.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  qom/object_interfaces.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)

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