Re: Re: [RFCv2 00/46] RFC: Generate parsexml/formatbuf functions based on directives
On 2020-12-05 at 02:55, DanielP. Berrangé wrote:
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
--
Thanks for reviewing this series.
For the first blocker, I think we can wait until April 2021, since we have spent a lot
time on it.
According to your comment, I think I can make some preparations for the initial merge
during
these 4~5 months.
(1) Implement some testcases for the generator to make sure its proper functions in the
future.
(2) Add some docs to explain and demonstrate this generator. My English is not well, so I
will
try my best to do it and I may need a bit long time.
(3) For the 'default' item of enums, I think we can add an extra directive to
direct the generator
how to generate code for enum, as I mentioned in the last email. Or we can continue to
talk about
other solutions.
(4) Next series will only include conversion about network, so that we can focus on the
docs,
testcases and this generator itself.
I plan to finish these jobs and post the next series in the next month, which will be
still a RFC.
So you and other people can have sufficent time to review and comment it and I will have
sufficent time to improve it :-)
Regards,
Shi Lei