[RFCv2 00/46] RFC: Generate parsexml/formatbuf functions based on directives

[Daniel P. Berrangé]

On Fri, Sep 04, 2020 at 11:34:52AM +0800, Shi Lei wrote:
> V1 here: [https://www.redhat.com/archives/libvir-list/2020-June/msg00357.html]
> 
> Differ from V1:
> 
>   * Move the generator into scripts/xmlgen and rename it 'xmlgen'.
> 
>   * Declare virXMLChildNode and virXMLChildNodeSet in libvirt_private.syms.
> 
>   * Replace VIR_FREE with g_free and VIR_ALLOC[_N] with g_new0.
> 
>   * Adjust virReportError to avoid unnecessary translation.
> 
>   * Remove the macro VIR_USED and use G_GNUC_UNUSED to declare arguments.
> 
>   * When parsing string member, assign value to it directly instead of
>     using middle variable.
> 
>   * Don't set libclang_path. Just use python-clang's default setting.
> 
>   * Use virEscapeString for escaping xml characters.
> 
>   * Enable directive 'genformat' with a parameter to support separation mode.
> 
>   * Add directive 'xmlswitch' and 'xmlgroup' to support discriminated unions. 
> 
>   * Allow directive 'array' and 'specified' to carry with a parameter,
>     which specifies its counterpart explicitly.
> 
>   * Enable directive 'xmlattr' with path.
> 
>   * Add directive 'formatflag' and 'formathook'.

I'm very sorry for the extremely long delay in reviewing this new
series.

Overall I like this series and would like to see how we can focus on
getting at least part of it merged.  It is interesting to see how it
is forcing a separation of the parsing and validation of data. I
also like that it is making the code more consistent in style.

The main blocker I think is that we need to temporarily commit the
generated files to git, and not run the generator during normal
builds, until we ditch RHEL-7 support in April 2021.  This should
be quite easy to deal with.

The next thing is that wierd need to insert a "_NONE" field in to
all the enums. I'm not understanding what reqiures this, but I think
we need to come up with a different solution to whatever the problem
is.  It looks like this only affects the DomainGraphics conversion
in your series. So we could start by only merging the Network conversion
patches, until we figure out a solution to avoid adding _NONE fields.

That's it for the main blockers to merge from my POV.

After that I think a priority is to get some test coverage of the
generator script, and to add the docs about the how the generator
annotations work. This can be done after the initial merge.

Then there's a large ongoing work to actually convert everything,
but there's no rush to finish that, as you've shown that we can
do it incrementally.

So if you want to re-post an update of the series with the few
blocking items addressed, then personally I'd look at trying to
merge that unless other people have objections they want to
raise that can't be solved after merging the initial support.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|