Re: [Qemu-devel] [PATCH v14 02/19] qapi-visit: Add visitor.type classification

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 04/13/2016 07:49 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> We have three classes of QAPI visitors: input, output, and dealloc.
>>> Currently, all implementations of these visitors have one thing in
>>> common based on their visitor type: the implementation used for the
>>> visit_type_enum() callback.  But since we plan to add more such
>>> common behavior, in relation to documenting and further refining
>>> the semantics, it makes more sense to have the visitor
>>> implementations advertise which class they belong to, so the common
>>> qapi-visit-core code can use that information in multiple places.
>>>
>>> For this patch, knowing the class of a visitor implementation lets
>>> us make input_type_enum() and output_type_enum() become static
>>> functions, by replacing the callback function Visitor.type_enum()
>>> with the simpler enum member Visitor.type.  Share a common
>>> assertion in qapi-visit-core as part of the refactoring.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>>
>
>>> +/* There are three classes of visitors; setting the class determines
>>> + * how QAPI enums are visited, as well as what additional restrictions
>>> + * can be asserted.  */
>>> +typedef enum VisitorType {
>>> +    VISITOR_INPUT,
>>> +    VISITOR_OUTPUT,
>>> +    VISITOR_DEALLOC,
>>> +} VisitorType;
>>> +
>>>  struct Visitor
>>>  {
>>>      /* Must be set */
>> 
>> I think we should explain what makes a visitor an input/output/dealloc
>> visitor.  Not necessarily in this patch, and not necessarily in this
>> place, just somewhere.  Right now, the information is scattered.
>
> 8/19 might be the patch that does just that.  We'll see what you think
> when you get further through the review.
>
>>> @@ -514,16 +516,6 @@ opts_visitor_new(const QemuOpts *opts)
>>>      ov->visitor.next_list  = &opts_next_list;
>>>      ov->visitor.end_list   = &opts_end_list;
>>>
>>> -    /* input_type_enum() covers both "normal" enums and union 
>>> discriminators.
>>> -     * The union discriminator field is always generated as "type"; it 
>>> should
>>> -     * match the "type" QemuOpt child of any QemuOpts.
>>> -     *
>>> -     * input_type_enum() will remove the looked-up key from the
>>> -     * "unprocessed_opts" hash even if the lookup fails, because the 
>>> removal is
>>> -     * done earlier in opts_type_str(). This should be harmless.
>>> -     */
>>> -    ov->visitor.type_enum = &input_type_enum;
>>> -
>> 
>> Hmm, this comment doesn't look worthless.  With its statement gone, I
>> guess it should move somewhere else.  What do you think?
>
> The first half of the comment is fluff.  The second half, about a
> looked-up key being removed from unprocessed_opts even if lookup fails,
> might be something I can move, but where? Maybe to the visit_type_enum()
> in qapi-visit-core.c, stating that an input visitor will visit the
> string even if conversion to enum fails? It really only affects what
> happens for an input visitor that has a visit_check_struct() (commit
> 14/19 of the series), but even then, we really only report an input
> visit failure regarding unvisited options if there was no earlier error
> - but the mere fact that visiting an enum type fails whether the string
> was present but not a valid enum value, or whether the string was not
> even present, means that we won't be reaching the visit_check_struct()
> to even care about errors about unvisited members.
>
> Maybe that means I just move the documentation into the commit message,
> and explain why the comment disappears (because a later patch will
> guarantee the semantics that we only care about reporting unvisited
> members in an input visitor only if all other visits are successful, so
> it doesn't matter on earlier failure whether we consumed or did not
> consume input).

The second half of the comment points out that visiting an enum can have
its side effect on unprocessed_opts even when the visit fails, namely
when input_type_enum() fails after visit_type_str() succeded.  Works as
long as visit_type_str() has no "unwelcome" side effects.

Your patch moves the enum handling into the visitor core.  I think to
replace the comment, we need two:

1. In the visitor implementation contract: state that visit_type_str()
may be called for enums, and that the enum visit may fail even when
visit_type_str() succeeds.  No need to go into input vs. output detail,
I think.

2. In opts_type_str(): point out that processed() gets called even
though the visit may still fail if its an enum, but it should be
harmless.