Re: [PATCH 1/2] qapi: introduce forwarding visitor

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 22/07/21 16:02, Markus Armbruster wrote:
>> Double-checking: the other fields are not accessible via this visitor.
>> Correct?
>
> Correct.
>
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>   include/qapi/forward-visitor.h    |  27 +++
>>>   qapi/meson.build                  |   1 +
>>>   qapi/qapi-forward-visitor.c       | 307 ++++++++++++++++++++++++++++++
>>>   tests/unit/meson.build            |   1 +
>>>   tests/unit/test-forward-visitor.c | 165 ++++++++++++++++
>>>   5 files changed, 501 insertions(+)
>>>   create mode 100644 include/qapi/forward-visitor.h
>>>   create mode 100644 qapi/qapi-forward-visitor.c
>>>   create mode 100644 tests/unit/test-forward-visitor.c
>> 
>> Missing: update of the big comment in include/qapi/visitor.h.  Can be
>> done on top.
>
> Also because I'm not sure what to add. :)
>
> This is not a fifth type of visitor, it's a wrapper for the existing
> types (two of them, input and output; the other two don't break
> horribly but make no sense either).

Unlike the other visitors, this one isn't of a fixed type.  I think
mentioning this would be nice.  Perhaps add to the paragraph

 * There are four kinds of visitors: input visitors (QObject, string,
 * and QemuOpts) parse an external representation and build the
 * corresponding QAPI object, output visitors (QObject and string)
 * take a QAPI object and generate an external representation, the
 * dealloc visitor takes a QAPI object (possibly partially
 * constructed) and recursively frees it, and the clone visitor
 * performs a deep clone of a QAPI object.

a sentence along the lines of "The forwarding visitor is special: it
wraps another visitor, and shares its type."

>>> +static bool forward_field_translate_name(ForwardFieldVisitor *v, const char **name,
>>> +                                         Error **errp)
>>> +{
>>> +    if (v->depth) {
>>> +        return true;
>>> +    }
>> 
>> Succeed when we're in a sub-struct.
>> 
>>> +    if (g_str_equal(*name, v->from)) {
>>> +        *name = v->to;
>>> +        return true;
>>> +    }
>> 
>> Succeed when we're in the root struct and @name is the alias name.
>> Replace the alias name by the real one.
>> 
>>> +    error_setg(errp, QERR_MISSING_PARAMETER, *name);
>>> +    return false;
>> 
>> Fail when we're in the root struct and @name is not the alias name.
>> 
>>> +}
>> 
>> Can you explain why you treat names in sub-structs differently than
>> names other than the alias name in the root struct?
>
> Taking the example of QOM alias properties, if the QOM property you're
> aliasing is a struct, its field names are irrelevant.  The caller may
> not even know what they are, as they are not part of the namespace (e.g.
> the toplevel QDict returned by keyval_parse) that is being modified.
>
> There are no aliased compound QOM properties that I can make a proper
> example with, unfortunately.

Since the intent is to forward *only* the alias, I wonder why we forward
*everything* when v->depth > 0.

Oh.  Is it because to get to v->depth > 0, we must have entered the
alias, so whatever we forward there must be members of the alias?

>>> +    /*
>>> +     * The name of alternates is reused when accessing the content,
>>> +     * so do not increase depth here.
>>> +     */
>> 
>> I understand why you don't increase @depth here (same reason
>> qobject-input-visitor.c doesn't qobject_input_push() here).  I don't
>> understand the comment :)
>
> See above: the alternate is not a struct; the names that are passed
> between start_alternate and end_alternate are within the same namespace
> as the toplevel field.

Yes.

> As to the comment, the idea is: if those calls used e.g. name == NULL,
> the depth would need to be increased, but the name will be the same one
> that was received by start_alternate.  Change to "The name passed to
> start_alternate is also used when accessing the content"?

Better.

>>> +Visitor *visitor_forward_field(Visitor *target, const char *from, const char *to)
>>> +{
>>> +    ForwardFieldVisitor *v = g_new0(ForwardFieldVisitor, 1);
>>> +
>>> +    v->visitor.type = target->type;
>> 
>> Do arbitrary types work?  Or is this limited to input and output
>> visitors?
>
> They don't crash, but they don't make sense because 1) they should not
> live outside qapi_clone and visit_free_* 2) they use NULL for the
> outermost name.

I'd prefer to restrict the forwarding visitor to the cases that make
sense and have test coverage.

>> Not forwarded: method .type_size().  Impact: visit_type_size() will call
>> the wrapped visitor's .type_uint64() instead of its .type_size().  The
>> two differ for the opts visitor, the keyval input visitor, the string
>> input visitor, and the string output visitor.
>
> Fixed, of course.  Incremental diff after my sig.

Looks good to me apart from rather long lines in block comments.
Best to wrap these around column 70, unless the wrapping obviously
reduces legibility.