Re: [PATCH 00/22] Cleanup and splitting of xl.cfg parsing

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.08.2023 17:33, Elliott Mitchell wrote:
> Is there a freeze on that I'm unaware of?  Is there so much traffic from
> other developers that smaller output ones are being missed?  I'm
> wondering about the initial revision of this series not getting any
> feedback:
> https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg01264.html
> 
> 
> Due to the lack of news on the first thread, I've done some looking to
> assess the feasibility.  The xl.cfg parser looks a bit jumbled.  Too many
> things are visible to too many places.  In general some pieces have
> really needed some TLC for a long time.
> 
> Note, some portions of this are semi-WIP.  The first 5 patches are simply
> overdue maintenance.  Not particularly urgent, but should probably be
> done.
> 
> Patch #06 is rather more urgent maintenance.  While it might not explode
> soon, that is a landmine.
> 
> Patch #07 is the first big issue.  The roles of the various headers had
> never been sorted out.  The underlying issue was more the contents of
> "libxlu_cfg_i.h" needed to be merged into "libxlu_cfg_y.h".  Bison 2.3
> may not have had the ability to embedded things into its header in 2010,
> but the functionality appears to have been present in Bison 3.3.
> 
> There is an issue of what level of indentation should be used in
> libxlu_cfg_y.y?  Normally the sections being added wouldn't be indented,
> but normally they would be directly in headers.  I'm unsure of which
> direction to go here.
> 
> Patch #08 seemed best to leave after #07 due to overlap and difficulty
> with reordering.  I'm a bit worried about the interfacing with flex.
> 
> 
> Then comes the issue of moving things out of libxlu_internal.h which
> should never have been there.  The XLU_Config* values should have been
> placed in libxlu_cfg_i.h instead.  Since I'm doing a thorough job,
> they're instead moving to libxlu_cfg.c.
> 
> I'm unsure splitting libxlu_cfg.c is worthwhile.  The resultant reusable
> libxlu_cfg.c is rather small.  Yet avoiding the need to reimplement the
> small portion is handy.
> 
> 
> Is the decision to keep in-tree copies of current Flex/Bison output still
> valid?  I would be awful tempted to remove them from main/master, even
> if copies are maintained on release branches.
> 
> 
> Elliott Mitchell (22):
>   tools/utils: cleanup formatting of libxlutil.h
>   tools/utils: rename "n" arguments to "key"
>   tools/utils: remove old declaration of xlu__cfg_yyparse()
>   tools/utils: enable all Bison warnings
>   tools/utils: update Bison parser directives
>   tools/utils: remove libxlu_cfg_i.h from libxlu_disk.c
>   tools/utils: merge contents of libxlu_cfg_i.h to libxlu_cfg_y.y
>   tools/utils: Bison: switch from name-prefix to api.prefix
>   tools/utils: move CfgParseContext to top of libxlu_cfg_y.y
>   tools/utils: move XLU_ConfigSetting to libxl_cfg.c
>   tools/utils: move XLU_ConfigValue to libxl_cfg.c
>   tools/utils: remove YYLTYPE definition from libxlu_internal.h
>   tools/utils: move XLU_ConfigList to libxl_cfg.c
>   tools/utils: introduce xlu_cfg_printf() function
>   tools/utils: move XLU_Config to libxlu_cfg.c
>   tools/utils: move XLU_Operation to libxlu_cfg_y.h
>   tools/utils: move setting free loop to xlu__cfg_set_free()
>   tools/utils: spread xlu_cfg_printf() over libxlu_cfg.c
>   tools/utils: add pointer to in-progress settings to CfgParseContext
>   tools/utils: add wrapper for readfile()/readdata() functions
>   tools/utils: add settings get function
>   tools/utils: break flex/bison parser away from main config file

Some of the patches looks to have been posted previously as a 7-patch
series. It would have been nice if therefore this one was marked as
v2, indicating in a revision log what the differences are. It appears
as if at least one out of those 7 earlier patches was dropped (or
maybe assimilated into another one).

Jan