| Message ID | 1453219845-30939-24-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Implement the new type_null() callback for the qmp input visitor. > While we don't yet have a use for this in qapi (the generator > will need some tweaks first), one usage is already envisioned: > when changing blockdev parameters, it would be nice to have a > difference between leaving a tuning parameter unchanged (omit > that parameter from the struct) and to explicitly reset the > parameter to its default without having to know what the default > value is (specify the parameter with an explicit null value, > which will require us to allow a qapi alternate that chooses > between the normal value and an explicit null). > > At any rate, we can test this without the use of generated qapi > by manually using visit_start_struct()/visit_end_struct(). Well, we test by calling visit_type_null() manually. We choose to wrap it in a visit_start_struct() ... visit_end_struct() pair, but that's detail. Actually, we do an unwrapped root visit first, and then a struct-wrapped visit. Suggest "by calling visit_type_null() manually." > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v9: new patch > --- > include/qapi/visitor-impl.h | 2 +- > qapi/qmp-input-visitor.c | 14 ++++++++++++++ > tests/test-qmp-input-visitor.c | 26 ++++++++++++++++++++++++++ > 3 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h > index 8705136..95408a5 100644 > --- a/include/qapi/visitor-impl.h > +++ b/include/qapi/visitor-impl.h > @@ -76,7 +76,7 @@ struct Visitor > void (*type_any)(Visitor *v, const char *name, QObject **obj, > Error **errp); > /* Must be provided to visit explicit null values (right now, only the > - * dealloc visitor supports this). */ > + * dealloc and qmp-input visitors support this). */ Will need updating. > void (*type_null)(Visitor *v, const char *name, Error **errp); > > /* May be NULL; most useful for input visitors. */ > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 597652c..ad23bec 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -315,6 +315,19 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj, > *obj = qobj; > } > > +static void qmp_input_type_null(Visitor *v, const char *name, Error **errp) > +{ > + QmpInputVisitor *qiv = to_qiv(v); > + QObject *qobj = qmp_input_get_object(qiv, name, true); > + > + if (qobject_type(qobj) == QTYPE_QNULL) { > + return; > + } > + > + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > + "null"); Recommend to put the error in the conditional: if (qobject_type(qobj) != QTYPE_QNULL) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "null"); } > +} > + > static void qmp_input_optional(Visitor *v, const char *name, bool *present) > { > QmpInputVisitor *qiv = to_qiv(v); > @@ -358,6 +371,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj) > v->visitor.type_str = qmp_input_type_str; > v->visitor.type_number = qmp_input_type_number; > v->visitor.type_any = qmp_input_type_any; > + v->visitor.type_null = qmp_input_type_null; > v->visitor.optional = qmp_input_optional; > v->visitor.get_next_type = qmp_input_get_next_type; > > diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c > index f6bd408..6489e4a 100644 > --- a/tests/test-qmp-input-visitor.c > +++ b/tests/test-qmp-input-visitor.c > @@ -278,6 +278,30 @@ static void test_visitor_in_any(TestInputVisitorData *data, > qobject_decref(res); > } > > +static void test_visitor_in_null(TestInputVisitorData *data, > + const void *unused) > +{ > + Visitor *v; > + QObject *null; > + > + v = visitor_input_test_init(data, "null"); > + visit_type_null(v, NULL, &error_abort); > + > + v = visitor_input_test_init(data, "{ 'a': null }"); > + visit_start_struct(v, NULL, NULL, 0, &error_abort); > + visit_type_null(v, "a", &error_abort); > + visit_end_struct(v, &error_abort); > + > + /* Check that qnull reference counting is sane: > + * 1 for global use, 1 for our qnull() use, and 1 still owned by 'v' > + * until it is torn down */ > + null = qnull(); > + g_assert(null->refcnt == 3); > + visitor_input_teardown(data, NULL); > + g_assert(null->refcnt == 2); > + qobject_decref(null); For other kinds of QObject, we leave the testing of reference counting to the check-qKIND.c, and don't bother with it when testing the visitors. Any particular reason to do null differently? > +} > + > static void test_visitor_in_union_flat(TestInputVisitorData *data, > const void *unused) > { > @@ -792,6 +816,8 @@ int main(int argc, char **argv) > &in_visitor_data, test_visitor_in_list); > input_visitor_test_add("/visitor/input/any", > &in_visitor_data, test_visitor_in_any); > + input_visitor_test_add("/visitor/input/null", > + &in_visitor_data, test_visitor_in_null); > input_visitor_test_add("/visitor/input/union-flat", > &in_visitor_data, test_visitor_in_union_flat); > input_visitor_test_add("/visitor/input/alternate",
On 01/22/2016 10:12 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Implement the new type_null() callback for the qmp input visitor. >> While we don't yet have a use for this in qapi (the generator >> will need some tweaks first), one usage is already envisioned: >> when changing blockdev parameters, it would be nice to have a >> difference between leaving a tuning parameter unchanged (omit >> that parameter from the struct) and to explicitly reset the >> parameter to its default without having to know what the default >> value is (specify the parameter with an explicit null value, >> which will require us to allow a qapi alternate that chooses >> between the normal value and an explicit null). >> >> At any rate, we can test this without the use of generated qapi >> by manually using visit_start_struct()/visit_end_struct(). > > Well, we test by calling visit_type_null() manually. We choose to wrap > it in a visit_start_struct() ... visit_end_struct() pair, but that's > detail. Actually, we do an unwrapped root visit first, and then a > struct-wrapped visit. > > Suggest "by calling visit_type_null() manually." > >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> --- >> +static void qmp_input_type_null(Visitor *v, const char *name, Error **errp) >> +{ >> + QmpInputVisitor *qiv = to_qiv(v); >> + QObject *qobj = qmp_input_get_object(qiv, name, true); >> + >> + if (qobject_type(qobj) == QTYPE_QNULL) { >> + return; >> + } >> + >> + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", >> + "null"); > > Recommend to put the error in the conditional: > > if (qobject_type(qobj) != QTYPE_QNULL) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "null"); > } Sure, I can reflow the logic. >> + /* Check that qnull reference counting is sane: >> + * 1 for global use, 1 for our qnull() use, and 1 still owned by 'v' >> + * until it is torn down */ >> + null = qnull(); >> + g_assert(null->refcnt == 3); >> + visitor_input_teardown(data, NULL); >> + g_assert(null->refcnt == 2); >> + qobject_decref(null); > > For other kinds of QObject, we leave the testing of reference counting > to the check-qKIND.c, and don't bother with it when testing the > visitors. Any particular reason to do null differently? Well, 19/37 added reference counting checks to test-qmp-output-visitor.c, and we don't have a check-qnull.c test yet. That, and the thing being checked here is that the visitor doesn't over- or under-reference the static qnull object (just checking qnull() without a visitor doesn't tell you if the visitor has any reference counting bugs). But maybe it is indeed worth writing a check-qnull.c file that does this work.
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 8705136..95408a5 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -76,7 +76,7 @@ struct Visitor void (*type_any)(Visitor *v, const char *name, QObject **obj, Error **errp); /* Must be provided to visit explicit null values (right now, only the - * dealloc visitor supports this). */ + * dealloc and qmp-input visitors support this). */ void (*type_null)(Visitor *v, const char *name, Error **errp); /* May be NULL; most useful for input visitors. */ diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index 597652c..ad23bec 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -315,6 +315,19 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj, *obj = qobj; } +static void qmp_input_type_null(Visitor *v, const char *name, Error **errp) +{ + QmpInputVisitor *qiv = to_qiv(v); + QObject *qobj = qmp_input_get_object(qiv, name, true); + + if (qobject_type(qobj) == QTYPE_QNULL) { + return; + } + + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", + "null"); +} + static void qmp_input_optional(Visitor *v, const char *name, bool *present) { QmpInputVisitor *qiv = to_qiv(v); @@ -358,6 +371,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj) v->visitor.type_str = qmp_input_type_str; v->visitor.type_number = qmp_input_type_number; v->visitor.type_any = qmp_input_type_any; + v->visitor.type_null = qmp_input_type_null; v->visitor.optional = qmp_input_optional; v->visitor.get_next_type = qmp_input_get_next_type; diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index f6bd408..6489e4a 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -278,6 +278,30 @@ static void test_visitor_in_any(TestInputVisitorData *data, qobject_decref(res); } +static void test_visitor_in_null(TestInputVisitorData *data, + const void *unused) +{ + Visitor *v; + QObject *null; + + v = visitor_input_test_init(data, "null"); + visit_type_null(v, NULL, &error_abort); + + v = visitor_input_test_init(data, "{ 'a': null }"); + visit_start_struct(v, NULL, NULL, 0, &error_abort); + visit_type_null(v, "a", &error_abort); + visit_end_struct(v, &error_abort); + + /* Check that qnull reference counting is sane: + * 1 for global use, 1 for our qnull() use, and 1 still owned by 'v' + * until it is torn down */ + null = qnull(); + g_assert(null->refcnt == 3); + visitor_input_teardown(data, NULL); + g_assert(null->refcnt == 2); + qobject_decref(null); +} + static void test_visitor_in_union_flat(TestInputVisitorData *data, const void *unused) { @@ -792,6 +816,8 @@ int main(int argc, char **argv) &in_visitor_data, test_visitor_in_list); input_visitor_test_add("/visitor/input/any", &in_visitor_data, test_visitor_in_any); + input_visitor_test_add("/visitor/input/null", + &in_visitor_data, test_visitor_in_null); input_visitor_test_add("/visitor/input/union-flat", &in_visitor_data, test_visitor_in_union_flat); input_visitor_test_add("/visitor/input/alternate",
Implement the new type_null() callback for the qmp input visitor. While we don't yet have a use for this in qapi (the generator will need some tweaks first), one usage is already envisioned: when changing blockdev parameters, it would be nice to have a difference between leaving a tuning parameter unchanged (omit that parameter from the struct) and to explicitly reset the parameter to its default without having to know what the default value is (specify the parameter with an explicit null value, which will require us to allow a qapi alternate that chooses between the normal value and an explicit null). At any rate, we can test this without the use of generated qapi by manually using visit_start_struct()/visit_end_struct(). Signed-off-by: Eric Blake <eblake@redhat.com> --- v9: new patch --- include/qapi/visitor-impl.h | 2 +- qapi/qmp-input-visitor.c | 14 ++++++++++++++ tests/test-qmp-input-visitor.c | 26 ++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 1 deletion(-)