Re: [Qemu-devel] [PATCH v9 23/37] qmp: Support explicit null during input visit

[Eric Blake]

On 01/22/2016 10:12 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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 <address@hidden>
>>
>> ---

>> +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