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

[Paolo Bonzini]

On 22/07/21 17:34, Markus Armbruster wrote:
> > 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

Ah okay, I didn't understand that paragraph referred to the actual 
visitors and not just the kinds in the enum.

> > > 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?

Yes, exactly.  v->depth is only nonzero after the name translation has 
succeeded (and until end_struct/end_list).

> > > > +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.

Yup, I had added an assertion in the incremental diff already.

> > > 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.

Thanks!

paolo