Re: [PATCH 01/11] qom: Reduce use of error_propagate()

Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> · Fri, 24 Sep 2021 16:23:43 +0300

24.09.2021 12:04, Kevin Wolf wrote:
ERRP_GUARD() makes debugging easier by making sure that &error_abort
still fails at the real origin of the error instead of
error_propagate().

Signed-off-by: Kevin Wolf <kwolf@xxxxxxxxxx>
---
  qom/object.c            |  7 +++----
  qom/object_interfaces.c | 17 ++++++-----------
  2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index e86cb05b84..6be710bc40 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1389,7 +1389,7 @@ bool object_property_get(Object *obj, const char *name, Visitor *v,
  bool object_property_set(Object *obj, const char *name, Visitor *v,
                           Error **errp)
  {
-    Error *err = NULL;
+    ERRP_GUARD();
      ObjectProperty *prop = object_property_find_err(obj, name, errp);
  
      if (prop == NULL) {
@@ -1400,9 +1400,8 @@ bool object_property_set(Object *obj, const char *name, Visitor *v,
          error_setg(errp, QERR_PERMISSION_DENIED);
          return false;
      }
-    prop->set(obj, v, name, prop->opaque, &err);
-    error_propagate(errp, err);
-    return !err;
+    prop->set(obj, v, name, prop->opaque, errp);
+    return !*errp;
  }

This is OK: prop->set doesn't return value, so we have to analyze errp to make our own return value.

  
  bool object_property_set_str(Object *obj, const char *name,
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index ad9b56b59a..80691e88cd 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -45,26 +45,21 @@ bool user_creatable_can_be_deleted(UserCreatable *uc)
  static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
                                               Visitor *v, Error **errp)
  {
+    ERRP_GUARD();
      const QDictEntry *e;
-    Error *local_err = NULL;
  
-    if (!visit_start_struct(v, NULL, NULL, 0, &local_err)) {
-        goto out;
+    if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
+        return;
      }
      for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-        if (!object_property_set(obj, e->key, v, &local_err)) {
+        if (!object_property_set(obj, e->key, v, errp)) {
              break;
          }
      }
-    if (!local_err) {
-        visit_check_struct(v, &local_err);
+    if (!*errp) {
+        visit_check_struct(v, errp);
      }
      visit_end_struct(v, NULL);
-
-out:
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
  }
  
  void object_set_properties_from_keyval(Object *obj, const QDict *qdict,


ERRP_GUARD() + dereferencing errp is good when we use functions which don't return any value. So we want to check is it fail or not and we have to analyze errp.

But that is not the case: all called functions follow modern recommendations of returning some value indicating failure together with setting errp.

I think, it's better not use ERRP_GUARD where it is not needed: in ideal world, all functions that may fail do return value, and we don't need neither error propagation, nor dereferencing errp. And don't need ERRP_GUARD.  ERRP_GUARD will be still needed to support error_prepend()/error_append() together with error_fatal, but again that's not the case.

Here I suggest something like this:

static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
                                             Visitor *v, Error **errp)
{
    const QDictEntry *e;
                                                                                 
    if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
        return;
    }
    for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
        if (!object_property_set(obj, e->key, v, errp)) {
            goto out;
        }
    }

    visit_check_struct(v, errp);

out:
    visit_end_struct(v, NULL);
}


--
Best regards,
Vladimir







