[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v14 02/19] qapi-visit: Add visitor.type classifi
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v14 02/19] qapi-visit: Add visitor.type classification |
Date: |
Fri, 15 Apr 2016 17:24:15 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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.
[Qemu-devel] [PATCH v14 12/19] spapr_drc: Expose 'null' in qom-get when there is no fdt, Eric Blake, 2016/04/08
[Qemu-devel] [PATCH v14 15/19] qapi-commands: Wrap argument visit in visit_start_struct, Eric Blake, 2016/04/08
[Qemu-devel] [PATCH v14 13/19] qmp: Tighten output visitor rules, Eric Blake, 2016/04/08
[Qemu-devel] [PATCH v14 16/19] qom: Wrap prop visit in visit_start_struct, Eric Blake, 2016/04/08