| Message ID | cover.1690990427.git.ehem+xen@m5p.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Cleanup and splitting of xl.cfg parsing | expand |
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
On Thu, Aug 03, 2023 at 10:35:53AM +0200, Jan Beulich wrote: > > 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). Indeed. Problem is several tags could potentially have been used. Should I have used all of them simultaneously? Should I have used only some of them? Which subset? Several were mildly adjusted, so it could have been marked "v2". No one responded at all to the previous round, so this could have been marked "RESEND". Yet the refinements and general changes are large enough for the series to be pretty distinct. I didn't know which way to go, so with no idea which option to choose the last one ended up winning out. Perhaps that was wrong yet I've still no feedback on the actual patches.
Hi Elliott, On 03/08/2023 17:13, Elliott Mitchell wrote: > On Thu, Aug 03, 2023 at 10:35:53AM +0200, Jan Beulich wrote: >> >> 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). > > Indeed. Problem is several tags could potentially have been used. > Should I have used all of them simultaneously? Should I have used only > some of them? Which subset? > > Several were mildly adjusted, so it could have been marked "v2". > > No one responded at all to the previous round, so this could have been > marked "RESEND". > > Yet the refinements and general changes are large enough for the series > to be pretty distinct. > > I didn't know which way to go, so with no idea which option to choose the > last one ended up winning out. Perhaps that was wrong yet I've still no > feedback on the actual patches. Not sure if this is related to the lack of answer. But I didn't receive any of your patches via xen-devel (I received your replies). Skimming through the bounce for the xenproject mail server, I noticed a lot of the following: host gmail-smtp-in.l.google.com [142.250.123.26] SMTP error from remote mail server after pipelined end of data: 550-5.7.1 This message is not RFC 5322 compliant. There are multiple Cc headers. 550-5.7.1 To reduce the amount of spam sent to Gmail, this message has been 550-5.7.1 blocked. Please visit 550 5.7.1 https://support.google.com/mail/?p=RfcMessageNonCompliant t7-20020a81e447000000b005839e8b595dsi12027284ywl.554 - gsmtp It might be possible that other mail server are not happy with your e-mails. Cheers,
On Thu, Aug 03, 2023 at 05:34:31PM +0100, Julien Grall wrote: > > On 03/08/2023 17:13, Elliott Mitchell wrote: > > > > I didn't know which way to go, so with no idea which option to choose the > > last one ended up winning out. Perhaps that was wrong yet I've still no > > feedback on the actual patches. > > Not sure if this is related to the lack of answer. But I didn't receive > any of your patches via xen-devel (I received your replies). Skimming > through the bounce for the xenproject mail server, I noticed a lot of > the following: > > host gmail-smtp-in.l.google.com [142.250.123.26] > SMTP error from remote mail server after pipelined end of data: > 550-5.7.1 This message is not RFC 5322 compliant. There are > multiple Cc headers. > 550-5.7.1 To reduce the amount of spam sent to Gmail, this message > has been > 550-5.7.1 blocked. Please visit > 550 5.7.1 > https://support.google.com/mail/?p=RfcMessageNonCompliant > t7-20020a81e447000000b005839e8b595dsi12027284ywl.554 - gsmtp That seems to be repotting a bug in `scripts/add_maintainers.pl`.
Hi, On 03/08/2023 18:45, Elliott Mitchell wrote: > On Thu, Aug 03, 2023 at 05:34:31PM +0100, Julien Grall wrote: >> >> On 03/08/2023 17:13, Elliott Mitchell wrote: >>> >>> I didn't know which way to go, so with no idea which option to choose the >>> last one ended up winning out. Perhaps that was wrong yet I've still no >>> feedback on the actual patches. >> >> Not sure if this is related to the lack of answer. But I didn't receive >> any of your patches via xen-devel (I received your replies). Skimming >> through the bounce for the xenproject mail server, I noticed a lot of >> the following: >> >> host gmail-smtp-in.l.google.com [142.250.123.26] >> SMTP error from remote mail server after pipelined end of data: >> 550-5.7.1 This message is not RFC 5322 compliant. There are >> multiple Cc headers. >> 550-5.7.1 To reduce the amount of spam sent to Gmail, this message >> has been >> 550-5.7.1 blocked. Please visit >> 550 5.7.1 >> https://support.google.com/mail/?p=RfcMessageNonCompliant >> t7-20020a81e447000000b005839e8b595dsi12027284ywl.554 - gsmtp > > That seems to be repotting a bug in `scripts/add_maintainers.pl`. I am curious to know why you think so? I have been using scripts/add_maintainers.pl for quite a while and so far never seen any of my e-mail blocked. My usual runes are: 42sh> scripts/add_maintainers.pl -d . 42sh> git-send-email *.patch What's yours? Cheers,
On 03.08.2023 18:13, Elliott Mitchell wrote: > On Thu, Aug 03, 2023 at 10:35:53AM +0200, Jan Beulich wrote: >> >> 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). > > Indeed. Problem is several tags could potentially have been used. > Should I have used all of them simultaneously? Should I have used only > some of them? Which subset? > > Several were mildly adjusted, so it could have been marked "v2". > > No one responded at all to the previous round, so this could have been > marked "RESEND". > > Yet the refinements and general changes are large enough for the series > to be pretty distinct. > > I didn't know which way to go, so with no idea which option to choose the > last one ended up winning out. Perhaps that was wrong yet I've still no > feedback on the actual patches. As, for this series, being just in the role of a committer, without clearly identifying that some earlier patches can now be dropped from the list of things which need monitoring, you're making my (in that regard) supposedly purely mechanical job harder (and presumably every other committer's as well). As to not getting feedback: Your posting (at least the cover letter) dates back to July 19. That's not all that long compared to other series. Plus if you were concerned, you could have pinged the respective maintainers. Jan
On Thu, Aug 03, 2023 at 06:48:33PM +0100, Julien Grall wrote: > Hi, > > On 03/08/2023 18:45, Elliott Mitchell wrote: > > On Thu, Aug 03, 2023 at 05:34:31PM +0100, Julien Grall wrote: > >> > >> Not sure if this is related to the lack of answer. But I didn't receive > >> any of your patches via xen-devel (I received your replies). Skimming > >> through the bounce for the xenproject mail server, I noticed a lot of > >> the following: > >> > >> host gmail-smtp-in.l.google.com [142.250.123.26] > >> SMTP error from remote mail server after pipelined end of data: > >> 550-5.7.1 This message is not RFC 5322 compliant. There are > >> multiple Cc headers. > >> 550-5.7.1 To reduce the amount of spam sent to Gmail, this message > >> has been > >> 550-5.7.1 blocked. Please visit > >> 550 5.7.1 > >> https://support.google.com/mail/?p=RfcMessageNonCompliant > >> t7-20020a81e447000000b005839e8b595dsi12027284ywl.554 - gsmtp > > > > That seems to be repotting a bug in `scripts/add_maintainers.pl`. > > I am curious to know why you think so? > > I have been using scripts/add_maintainers.pl for quite a while and so > far never seen any of my e-mail blocked. > > My usual runes are: > > 42sh> scripts/add_maintainers.pl -d . > 42sh> git-send-email *.patch > > What's yours? Final steps are: scp -r snd mailserver: for f in snd/0*.patch do sendmail -t < "$f" sleep 45 done Issue with `git send-email` is it really needs all the setup of a MUA, and I prefer to keep `git` on a distinct host.
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 tools/include/libxlutil.h | 31 +- tools/libs/util/Makefile | 3 +- tools/libs/util/libxlu_cfg.c | 679 ++---------------- .../util/{libxlu_cfg.c => libxlu_cfg_i.c} | 196 +++-- tools/libs/util/libxlu_cfg_i.h | 59 -- tools/libs/util/libxlu_cfg_l.l | 2 +- tools/libs/util/libxlu_cfg_y.y | 58 +- tools/libs/util/libxlu_disk.c | 15 +- tools/libs/util/libxlu_internal.h | 59 +- tools/libs/util/libxlu_pci.c | 11 +- tools/libs/util/libxlu_vif.c | 4 +- 11 files changed, 223 insertions(+), 894 deletions(-) copy tools/libs/util/{libxlu_cfg.c => libxlu_cfg_i.c} (78%) delete mode 100644 tools/libs/util/libxlu_cfg_i.h