Re: [PATCH 01/11] qom: Reduce use of error_propagate()
On Fri, Sep 24, 2021 at 11:04:17AM +0200, Kevin Wolf wrote: > ERRP_GUARD() makes debugging easier by making sure that _abort > still fails at the real origin of the error instead of > error_propagate(). > > Signed-off-by: Kevin Wolf > --- > qom/object.c| 7 +++ > qom/object_interfaces.c | 17 ++--- > 2 files changed, 9 insertions(+), 15 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 01/11] qom: Reduce use of error_propagate()
Kevin Wolf writes: > ERRP_GUARD() makes debugging easier by making sure that _abort > still fails at the real origin of the error instead of > error_propagate(). > > Signed-off-by: Kevin Wolf Yes. The code you patch uses error_propagate() to work around functions not returning distinct error values. error.h's big comment recommends such return values, but recommendations don't update code, patches do. Until then, ERRP_GUARD() is clearly a better crutch than error_propagate(). Reviewed-by: Markus Armbruster
Re: [PATCH 01/11] qom: Reduce use of error_propagate()
24.09.2021 12:04, Kevin Wolf wrote:
ERRP_GUARD() makes debugging easier by making sure that _abort
still fails at the real origin of the error instead of
error_propagate().
Signed-off-by: Kevin Wolf
---
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, );
-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, _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, _err)) {
+if (!object_property_set(obj, e->key, v, errp)) {
break;
}
}
-if (!local_err) {
-visit_check_struct(v, _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
[PATCH 01/11] qom: Reduce use of error_propagate()
ERRP_GUARD() makes debugging easier by making sure that _abort
still fails at the real origin of the error instead of
error_propagate().
Signed-off-by: Kevin Wolf
---
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, );
-error_propagate(errp, err);
-return !err;
+prop->set(obj, v, name, prop->opaque, errp);
+return !*errp;
}
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, _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, _err)) {
+if (!object_property_set(obj, e->key, v, errp)) {
break;
}
}
-if (!local_err) {
-visit_check_struct(v, _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,
--
2.31.1
