Eric Blake <ebl...@redhat.com> writes: > On 04/13/2016 07:49 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com> >>> > >>> +/* 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.