Paolo Bonzini <pbonz...@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 <pbonz...@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.
