Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes:
> 02.07.2020 18:49, 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 | 67 ++++++++------
>> 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, 452 insertions(+), 355 deletions(-)
>>
>> 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
>> @@ -1408,42 +1408,38 @@ Example:
>> #include "example-qapi-types.h"
>> - void visit_type_UserDefOne_members(Visitor *v, UserDefOne
>> *obj, Error **errp);
>> - void visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne
>> **obj, Error **errp);
>> - void visit_type_UserDefOneList(Visitor *v, const char *name,
>> UserDefOneList **obj, Error **errp);
>> + bool visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error
>> **errp);
>> + bool visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne
>> **obj, Error **errp);
>> + bool visit_type_UserDefOneList(Visitor *v, const char *name,
>> UserDefOneList **obj, Error **errp);
>> - 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);
>> #endif /* EXAMPLE_QAPI_VISIT_H */
>> $ cat qapi-generated/example-qapi-visit.c
>
> should it be tests/test-qapi-visit.c ? [preexisting, don't care]
No, that one's generated from tests/qapi-schema/qapi-schema-test.json,
while this one is generated from example-schema.json.
>> [Uninteresting stuff omitted...]
>> - 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;
>> }
>> }
>> -
>> - out:
>> error_propagate(errp, err);
>> + return !err;
>
> Looks weird with redundant err variable.. Still works. I see, after the whole
> series it is handled, so, OK.
Eric spotted this in v1, and asked for an explanation. Perhaps I should
have added a note to my commit message then.
This patch has a narrow scope: it does just enough to make the functions
return bool. The result is a bit weird in places. PATCH 39 will
straighten it out.
We can't drop the @err and error_propagate() here just yet, because the
generator can still generate code that needs it, just not for this
particular input.
>> }
>> - 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;
>
> [..]
>
>> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
>> index 5fe0276c1c..6d8f4b6928 100644
>> --- a/qapi/opts-visitor.c
>> +++ b/qapi/opts-visitor.c
>> @@ -133,7 +133,7 @@ opts_visitor_insert(GHashTable *unprocessed_opts, const
>> QemuOpt *opt)
>> }
>>
>
> [..]
>
>> @@ -221,7 +224,7 @@ lookup_distinct(const OptsVisitor *ov, const
>> char *name, Error **errp)
>> }
>> -static void
>> +static bool
>> opts_start_list(Visitor *v, const char *name, GenericList **list, size_t
>> size,
>> Error **errp)
>> {
>> @@ -238,6 +241,7 @@ opts_start_list(Visitor *v, const char *name,
>> GenericList **list, size_t size,
>> } else {
>> *list = NULL;
>
> should return false here, as errp is set.
Falcon eyes! I'll fix it.
>> }
>> + return true;
>> }
>> @@ -285,13 +289,14 @@ opts_next_list(Visitor *v, GenericList
>> *tail, size_t size)
>> }
>>
>
> [..]
>
>> diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c
>> index daab6819b4..5a54bd593f 100644
>> --- a/qapi/qapi-clone-visitor.c
>> +++ b/qapi/qapi-clone-visitor.c
>> @@ -24,7 +24,7 @@ static QapiCloneVisitor *to_qcv(Visitor *v)
>> return container_of(v, QapiCloneVisitor, visitor);
>> }
>> -static void qapi_clone_start_struct(Visitor *v, const char *name,
>> void **obj,
>> +static bool qapi_clone_start_struct(Visitor *v, const char *name, void
>> **obj,
>> size_t size, Error **errp)
>> {
>> QapiCloneVisitor *qcv = to_qcv(v);
>> @@ -34,11 +34,12 @@ static void qapi_clone_start_struct(Visitor *v, const
>> char *name, void **obj,
>> /* Only possible when visiting an alternate's object
>> * branch. Nothing further to do here, since the earlier
>> * visit_start_alternate() already copied memory. */
>> - return;
>> + return true;
>> }
>> *obj = g_memdup(*obj, size);
>> qcv->depth++;
>> + return true;
>> }
>> static void qapi_clone_end(Visitor *v, void **obj)
>> @@ -51,11 +52,12 @@ static void qapi_clone_end(Visitor *v, void **obj)
>> }
>> }
>> -static void qapi_clone_start_list(Visitor *v, const char *name,
>> +static bool qapi_clone_start_list(Visitor *v, const char *name,
>> GenericList **listp, size_t size,
>> Error **errp)
>> {
>> qapi_clone_start_struct(v, name, (void **)listp, size, errp);
>> + return true;
>
> should be return qapi_clone_start_struct(v, name, (void **)listp, size, errp);
You're right.
qapi_clone_start_struct() only ever returns true, but relying on that
would be unclean.
>> }
>> static GenericList *qapi_clone_next_list(Visitor *v, GenericList
>> *tail,
>> @@ -69,45 +71,49 @@ static GenericList *qapi_clone_next_list(Visitor *v,
>> GenericList *tail,
>> return tail->next;
>> }
>> -static void qapi_clone_start_alternate(Visitor *v, const char
>> *name,
>> +static bool qapi_clone_start_alternate(Visitor *v, const char *name,
>> GenericAlternate **obj, size_t size,
>> Error **errp)
>> {
>> qapi_clone_start_struct(v, name, (void **)obj, size, errp);
>> + return true;
>
> similar here
Right again.
>> }
>> -static void qapi_clone_type_int64(Visitor *v, const char *name,
>> int64_t *obj,
>> - Error **errp)
>> -{
>> - QapiCloneVisitor *qcv = to_qcv(v);
>> -
>> - assert(qcv->depth);
>> - /* Value was already cloned by g_memdup() */
>> -}
>
>
> With three return values fixed:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
Thanks!
