On 6/24/20 11:43 AM, Markus Armbruster wrote:
See recent commit "error: Document Error API usage rules" for
rationale.
Signed-off-by: Markus Armbruster <arm...@redhat.com>
---
docs/devel/qapi-code-gen.txt | 51 +++++------
include/qapi/clone-visitor.h | 8 +-
include/qapi/visitor-impl.h | 26 +++---
include/qapi/visitor.h | 102 ++++++++++++---------
audio/audio_legacy.c | 15 ++--
qapi/opts-visitor.c | 58 +++++++-----
qapi/qapi-clone-visitor.c | 33 ++++---
qapi/qapi-dealloc-visitor.c | 27 ++++--
qapi/qapi-visit-core.c | 165 ++++++++++++++++++----------------
qapi/qobject-input-visitor.c | 109 +++++++++++++---------
qapi/qobject-output-visitor.c | 27 ++++--
qapi/string-input-visitor.c | 62 +++++++------
qapi/string-output-visitor.c | 32 ++++---
scripts/qapi/visit.py | 58 +++++-------
14 files changed, 435 insertions(+), 338 deletions(-)
Hefty, but I don't see a sane way to split it further.
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index a7794ef658..9bfc57063c 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
- void visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error
**errp)
+ bool visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error
**errp)
{
Error *err = NULL;
- visit_type_int(v, "integer", &obj->integer, &err);
- if (err) {
- goto out;
+ if (!visit_type_int(v, "integer", &obj->integer, errp)) {
+ return false;
}
if (visit_optional(v, "string", &obj->has_string)) {
- visit_type_str(v, "string", &obj->string, &err);
- if (err) {
- goto out;
+ if (!visit_type_str(v, "string", &obj->string, errp)) {
+ return false;
}
}
Is this worth compressing two 'if's into one:
if (visit_optional(...) &&
!visit_type_str(...)) {
return false;
}
-
- out:
error_propagate(errp, err);
+ return !err;
Now that 'err' is never anything but NULL, why aren't you dropping the
error_propagate() and merely using 'return true;'?
}
- void visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj, Error **errp)
+ bool visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj,
Error **errp)
{
Error *err = NULL;
- visit_start_struct(v, name, (void **)obj, sizeof(UserDefOne), &err);
- if (err) {
- goto out;
+ if (!visit_start_struct(v, name, (void **)obj, sizeof(UserDefOne),
errp)) {
+ return false;
}
if (!*obj) {
/* incomplete */
@@ -1461,19 +1457,18 @@ Example:
Adding context:
assert(visit_is_dealloc(v));
goto out_obj;
}
visit_type_UserDefOne_members(v, *obj, &err);
if (err) {
goto out_obj;
Should this be:
if (!visit_type_UserDefOne_members(v, *obj, &err)) {
goto out_obj;
}
visit_check_struct(v, &err);
out_obj:
visit_end_struct(v, (void **)obj);
if (err && visit_is_input(v)) {
qapi_free_UserDefOne(*obj);
*obj = NULL;
}
- out:
error_propagate(errp, err);
+ return !err;
Here, err is still used by out_obj:, so this one is fine.
}
- void visit_type_UserDefOneList(Visitor *v, const char *name, UserDefOneList **obj, Error **errp)
+ bool visit_type_UserDefOneList(Visitor *v, const char *name,
UserDefOneList **obj, Error **errp)
{
Error *err = NULL;
UserDefOneList *tail;
size_t size = sizeof(**obj);
- visit_start_list(v, name, (GenericList **)obj, size, &err);
- if (err) {
- goto out;
+ if (!visit_start_list(v, name, (GenericList **)obj, size, errp)) {
+ return false;
}
for (tail = *obj; tail;
@@ -1492,21 +1487,19 @@ Example:
Adding context:
tail = (UserDefOneList *)visit_next_list(v, (GenericList *)tail,
size)) {
visit_type_UserDefOne(v, NULL, &tail->value, &err);
if (err) {
break;
}
Should this be:
if (visit_type_UserDefOne(...)) {
break;
}
if (!err) {
visit_check_list(v, &err);
}
visit_end_list(v, (void **)obj);
if (err && visit_is_input(v)) {
qapi_free_UserDefOneList(*obj);
*obj = NULL;
}
- out:
error_propagate(errp, err);
+ return !err;
}
Again, err is still used, so this one is fine.
- void visit_type_q_obj_my_command_arg_members(Visitor *v, q_obj_my_command_arg *obj, Error **errp)
+ bool visit_type_q_obj_my_command_arg_members(Visitor *v,
q_obj_my_command_arg *obj, Error **errp)
{
Error *err = NULL;
- visit_type_UserDefOneList(v, "arg1", &obj->arg1, &err);
- if (err) {
- goto out;
+ if (!visit_type_UserDefOneList(v, "arg1", &obj->arg1, errp)) {
+ return false;
}
-
- out:
error_propagate(errp, err);
+ return !err;
}
But this is another one where err is unused.
[Uninteresting stuff omitted...]
diff --git a/include/qapi/clone-visitor.h b/include/qapi/clone-visitor.h
index 5b665ee38c..adf9a788e2 100644
--- a/include/qapi/clone-visitor.h
+++ b/include/qapi/visitor.h
@@ -286,6 +284,8 @@ void visit_free(Visitor *v);
* On failure, set *@obj to NULL and store an error through @errp.
* Can happen only when @v is an input visitor.
*
+ * Return true on succes, false on failure.
success
(copied several times in this file)
+++ b/qapi/qapi-clone-visitor.c
-static void qapi_clone_type_int64(Visitor *v, const char *name, int64_t *obj,
+static bool qapi_clone_type_int64(Visitor *v, const char *name, int64_t *obj,
Error **errp)
Pre-existing indentation glitch that you could fix while here.
{
QapiCloneVisitor *qcv = to_qcv(v);
assert(qcv->depth);
/* Value was already cloned by g_memdup() */
+ return true;
}
-static void qapi_clone_type_uint64(Visitor *v, const char *name,
+static bool qapi_clone_type_uint64(Visitor *v, const char *name,
uint64_t *obj, Error **errp)
Ditto for several more functions in this file.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
