Eric Blake <ebl...@redhat.com> writes: > We have three classes of QAPI visitors: input, output, and dealloc. > Currently, all implementations of these visitors have one thing in > common based on their visitor type: the implementation used for the > visit_type_enum() callback. But since we plan to add more such > common behavior, in relation to documenting and further refining > the semantics, it makes more sense to have the visitor > implementations advertise which class they belong to, so the common > qapi-visit-core code can use that information in multiple places. > > For this patch, knowing the class of a visitor implementation lets > us make input_type_enum() and output_type_enum() become static > functions, by replacing the callback function Visitor.type_enum() > with the simpler enum member Visitor.type. Share a common > assertion in qapi-visit-core as part of the refactoring. > > Signed-off-by: Eric Blake <ebl...@redhat.com> > > --- > v14: no change > v13: no change > v12: new patch > --- > include/qapi/visitor-impl.h | 21 ++++++++++++--------- > qapi/qapi-visit-core.c | 28 +++++++++++++++------------- > qapi/opts-visitor.c | 12 ++---------- > qapi/qapi-dealloc-visitor.c | 7 +------ > qapi/qmp-input-visitor.c | 2 +- > qapi/qmp-output-visitor.c | 2 +- > qapi/string-input-visitor.c | 2 +- > qapi/string-output-visitor.c | 2 +- > 8 files changed, 34 insertions(+), 42 deletions(-) > > diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h > index 2bd8f29..228a2a6 100644 > --- a/include/qapi/visitor-impl.h > +++ b/include/qapi/visitor-impl.h > @@ -14,6 +14,15 @@ > > #include "qapi/visitor.h" > > +/* There are three classes of visitors; setting the class determines > + * how QAPI enums are visited, as well as what additional restrictions > + * can be asserted. */ > +typedef enum VisitorType { > + VISITOR_INPUT, > + VISITOR_OUTPUT, > + VISITOR_DEALLOC, > +} VisitorType; > + > struct Visitor > { > /* Must be set */
I think we should explain what makes a visitor an input/output/dealloc visitor. Not necessarily in this patch, and not necessarily in this place, just somewhere. Right now, the information is scattered. > @@ -36,10 +45,6 @@ struct Visitor > void (*end_alternate)(Visitor *v); > > /* Must be set. */ > - void (*type_enum)(Visitor *v, const char *name, int *obj, > - const char *const strings[], Error **errp); > - > - /* Must be set. */ > void (*type_int64)(Visitor *v, const char *name, int64_t *obj, > Error **errp); > /* Must be set. */ > @@ -58,11 +63,9 @@ struct Visitor > > /* May be NULL; most useful for input visitors. */ > void (*optional)(Visitor *v, const char *name, bool *present); > + > + /* Must be set. */ > + VisitorType type; > }; > > -void input_type_enum(Visitor *v, const char *name, int *obj, > - const char *const strings[], Error **errp); > -void output_type_enum(Visitor *v, const char *name, int *obj, > - const char *const strings[], Error **errp); > - > #endif > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index fa680c9..3cd7edc 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -72,12 +72,6 @@ bool visit_optional(Visitor *v, const char *name, bool > *present) > return *present; > } > > -void visit_type_enum(Visitor *v, const char *name, int *obj, > - const char *const strings[], Error **errp) > -{ > - v->type_enum(v, name, obj, strings, errp); > -} > - > void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp) > { > v->type_int64(v, name, obj, errp); > @@ -208,14 +202,13 @@ void visit_type_any(Visitor *v, const char *name, > QObject **obj, Error **errp) > v->type_any(v, name, obj, errp); > } > > -void output_type_enum(Visitor *v, const char *name, int *obj, > - const char *const strings[], Error **errp) > +static void output_type_enum(Visitor *v, const char *name, int *obj, > + const char *const strings[], Error **errp) > { > int i = 0; > int value = *obj; > char *enum_str; > > - assert(strings); > while (strings[i++] != NULL); > if (value < 0 || value >= i - 1) { > error_setg(errp, QERR_INVALID_PARAMETER, name ? name : "null"); > @@ -226,15 +219,13 @@ void output_type_enum(Visitor *v, const char *name, int > *obj, > visit_type_str(v, name, &enum_str, errp); > } > > -void input_type_enum(Visitor *v, const char *name, int *obj, > - const char *const strings[], Error **errp) > +static void input_type_enum(Visitor *v, const char *name, int *obj, > + const char *const strings[], Error **errp) > { > Error *local_err = NULL; > int64_t value = 0; > char *enum_str; > > - assert(strings); > - > visit_type_str(v, name, &enum_str, &local_err); > if (local_err) { > error_propagate(errp, local_err); > @@ -257,3 +248,14 @@ void input_type_enum(Visitor *v, const char *name, int > *obj, > g_free(enum_str); > *obj = value; > } > + > +void visit_type_enum(Visitor *v, const char *name, int *obj, > + const char *const strings[], Error **errp) > +{ > + assert(strings); > + if (v->type == VISITOR_INPUT) { > + input_type_enum(v, name, obj, strings, errp); > + } else if (v->type == VISITOR_OUTPUT) { > + output_type_enum(v, name, obj, strings, errp); > + } > +} > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c > index 602f260..f98cf2e 100644 > --- a/qapi/opts-visitor.c > +++ b/qapi/opts-visitor.c > @@ -507,6 +507,8 @@ opts_visitor_new(const QemuOpts *opts) > > ov = g_malloc0(sizeof *ov); > > + ov->visitor.type = VISITOR_INPUT; > + > ov->visitor.start_struct = &opts_start_struct; > ov->visitor.end_struct = &opts_end_struct; > > @@ -514,16 +516,6 @@ opts_visitor_new(const QemuOpts *opts) > ov->visitor.next_list = &opts_next_list; > ov->visitor.end_list = &opts_end_list; > > - /* input_type_enum() covers both "normal" enums and union discriminators. > - * The union discriminator field is always generated as "type"; it should > - * match the "type" QemuOpt child of any QemuOpts. > - * > - * input_type_enum() will remove the looked-up key from the > - * "unprocessed_opts" hash even if the lookup fails, because the removal > is > - * done earlier in opts_type_str(). This should be harmless. > - */ > - ov->visitor.type_enum = &input_type_enum; > - Hmm, this comment doesn't look worthless. With its statement gone, I guess it should move somewhere else. What do you think? > ov->visitor.type_int64 = &opts_type_int64; > ov->visitor.type_uint64 = &opts_type_uint64; > ov->visitor.type_size = &opts_type_size; > diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c > index 6922179..c19a459 100644 > --- a/qapi/qapi-dealloc-visitor.c > +++ b/qapi/qapi-dealloc-visitor.c > @@ -163,11 +163,6 @@ static void qapi_dealloc_type_anything(Visitor *v, const > char *name, > } > } > > -static void qapi_dealloc_type_enum(Visitor *v, const char *name, int *obj, > - const char * const strings[], Error > **errp) > -{ > -} > - > Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v) > { > return &v->visitor; > @@ -184,6 +179,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void) > > v = g_malloc0(sizeof(*v)); > > + v->visitor.type = VISITOR_DEALLOC; > v->visitor.start_struct = qapi_dealloc_start_struct; > v->visitor.end_struct = qapi_dealloc_end_struct; > v->visitor.start_alternate = qapi_dealloc_start_alternate; > @@ -191,7 +187,6 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void) > v->visitor.start_list = qapi_dealloc_start_list; > v->visitor.next_list = qapi_dealloc_next_list; > v->visitor.end_list = qapi_dealloc_end_list; > - v->visitor.type_enum = qapi_dealloc_type_enum; > v->visitor.type_int64 = qapi_dealloc_type_int64; > v->visitor.type_uint64 = qapi_dealloc_type_uint64; > v->visitor.type_bool = qapi_dealloc_type_bool; > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 7cd1b77..02d4233 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -339,13 +339,13 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj) > > v = g_malloc0(sizeof(*v)); > > + v->visitor.type = VISITOR_INPUT; > v->visitor.start_struct = qmp_input_start_struct; > v->visitor.end_struct = qmp_input_end_struct; > v->visitor.start_list = qmp_input_start_list; > v->visitor.next_list = qmp_input_next_list; > v->visitor.end_list = qmp_input_end_list; > v->visitor.start_alternate = qmp_input_start_alternate; > - v->visitor.type_enum = input_type_enum; > v->visitor.type_int64 = qmp_input_type_int64; > v->visitor.type_uint64 = qmp_input_type_uint64; > v->visitor.type_bool = qmp_input_type_bool; > diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c > index d44c676..1f2a7ba 100644 > --- a/qapi/qmp-output-visitor.c > +++ b/qapi/qmp-output-visitor.c > @@ -234,12 +234,12 @@ QmpOutputVisitor *qmp_output_visitor_new(void) > > v = g_malloc0(sizeof(*v)); > > + v->visitor.type = VISITOR_OUTPUT; > v->visitor.start_struct = qmp_output_start_struct; > v->visitor.end_struct = qmp_output_end_struct; > v->visitor.start_list = qmp_output_start_list; > v->visitor.next_list = qmp_output_next_list; > v->visitor.end_list = qmp_output_end_list; > - v->visitor.type_enum = output_type_enum; > v->visitor.type_int64 = qmp_output_type_int64; > v->visitor.type_uint64 = qmp_output_type_uint64; > v->visitor.type_bool = qmp_output_type_bool; > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index ab12953..d604575 100644 > --- a/qapi/string-input-visitor.c > +++ b/qapi/string-input-visitor.c > @@ -348,7 +348,7 @@ StringInputVisitor *string_input_visitor_new(const char > *str) > > v = g_malloc0(sizeof(*v)); > > - v->visitor.type_enum = input_type_enum; > + v->visitor.type = VISITOR_INPUT; > v->visitor.type_int64 = parse_type_int64; > v->visitor.type_uint64 = parse_type_uint64; > v->visitor.type_size = parse_type_size; > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c > index c2e5c5b..0d44d7e 100644 > --- a/qapi/string-output-visitor.c > +++ b/qapi/string-output-visitor.c > @@ -351,7 +351,7 @@ StringOutputVisitor *string_output_visitor_new(bool human) > > v->string = g_string_new(NULL); > v->human = human; > - v->visitor.type_enum = output_type_enum; > + v->visitor.type = VISITOR_OUTPUT; > v->visitor.type_int64 = print_type_int64; > v->visitor.type_uint64 = print_type_uint64; > v->visitor.type_size = print_type_size;