| Message ID | 20210924090427.9218-2-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qdev: Add JSON -device and fix QMP device_add | expand |
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@redhat.com> > --- > 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); }
Kevin Wolf <kwolf@redhat.com> writes: > 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@redhat.com> 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 <armbru@redhat.com>
On Fri, Sep 24, 2021 at 11:04:17AM +0200, 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@redhat.com> > --- > qom/object.c | 7 +++---- > qom/object_interfaces.c | 17 ++++++----------- > 2 files changed, 9 insertions(+), 15 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
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; } 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() 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@redhat.com> --- qom/object.c | 7 +++---- qom/object_interfaces.c | 17 ++++++----------- 2 files changed, 9 insertions(+), 15 deletions(-)