On 01/22/2016 10:12 AM, Markus Armbruster wrote: > Eric Blake <ebl...@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 <ebl...@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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature