Bug 244640 (lua_loader_read-conf) - [Patch][Lua loader] Implement missing "read-conf" 4th command
Summary: [Patch][Lua loader] Implement missing "read-conf" 4th command
Status: Closed FIXED
Alias: lua_loader_read-conf
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Kyle Evans
URL:
Keywords: loader, patch-ready
Depends on:
Blocks:
 
Reported: 2020-03-06 18:15 UTC by Olivier Certner
Modified: 2020-05-09 08:48 UTC (History)
2 users (show)

See Also:
olivier.freebsd: mfc-stable12?
olivier.freebsd: mfc-stable11?


Attachments
Patch adding "read-conf" (419 bytes, patch)
2020-03-06 18:15 UTC, Olivier Certner
no flags Details | Diff
Replace "readConfFiles" by "readConf", simplifications, fix corner case (4.89 KB, patch)
2020-04-29 12:28 UTC, Olivier Certner
no flags Details | Diff
MFC to 12 (6.40 KB, patch)
2020-05-02 20:12 UTC, Olivier Certner
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Olivier Certner 2020-03-06 18:15:28 UTC
Created attachment 212202 [details]
Patch adding "read-conf"

Please find attached a patch implementing "read-conf" in the Lua loader, as defined in loader.4th(8). Is currently in use with `head` and `stable/12` machines without problems.

For the record:

1. As this command is not built-in, it would normally have to obey Lua syntax, which forbids calling functions having a `-` in their name with the common function call syntax. However, .

2. As a workaround, one could type those two lines:
`config=require("config")`
`config.processFile "<conf_file_name>"`
but this is annoying and not officially documented (most probably on purpose).

Thank you.
Comment 1 Olivier Certner 2020-03-06 18:24:22 UTC
Sorry, missing text in 1. in original submission. Should read:

1. As this command is not built-in, it would normally have to obey Lua syntax, which forbids calling functions having a `-` in their name with the common function call syntax. However, Lua loader's specific machinery handles the first string in the command as the name of a function to call. So it is indeed possible to use "read-conf" by typing:
`"read-conf" "<conf_file_name>"`
at the loader prompt. Actually, "boot-conf" was already defined and could be used this way.
Comment 2 commit-hook freebsd_committer 2020-04-28 02:05:23 UTC
A commit references this bug:

Author: kevans
Date: Tue Apr 28 02:04:52 UTC 2020
New revision: 360422
URL: https://svnweb.freebsd.org/changeset/base/360422

Log:
  lualoader: cli: add read-conf

  This is a straightforward match to the command used by many in forthloader;
  it uses the newly-exported config.readConfFiles() to make sure that any
  loader_conf_files gets done as appropriate.

  PR:		244640
  Submitted by:	Olivier Certner <olivier freebsd free fr>
  MFC after:	3 days

Changes:
  head/stand/lua/cli.lua
Comment 3 Kyle Evans freebsd_committer 2020-04-28 02:09:23 UTC
Apologies for the much-too-long delay on this. =-( I have done the refactoring I mentioned needed to be done to make sure loader_conf_files gets processed correctly, though I no longer recall what other nuances I thought needed to be fixed.

I'll MFC the batch in the next ~3/4 days so that it's available in both the 11.4 preview and 12.2.
Comment 4 Olivier Certner 2020-04-28 16:11:17 UTC
Hey Kyle, thanks. Long delay yes, but I didn't take the time either (busy with lots of other things).

After reviewing the latest commits (3604{20..23,25,27}), I think that some corner case may not be well handled. Also, looking at the end result, it seems the related functions could be simplified a lot (under my maybe-limited understanding of the whole thing).

So I'm going to take a shot at it and provide you an additional patch tonight or tomorrow, in the hope that you can then vet it, and in particular tell me if I missed some use case that would prevent some of the simplifications.
Comment 5 Kyle Evans freebsd_committer 2020-04-28 16:14:15 UTC
(In reply to Olivier Certner from comment #4)

Sure thing- iteration is good. =-)
Comment 6 Olivier Certner 2020-04-29 12:28:21 UTC
Created attachment 213909 [details]
Replace "readConfFiles" by "readConf", simplifications, fix corner case

The corner case I spotted is that some "read-conf" issued after "loader_conf_files" has been set fails to reload these files if the initial file to read contains "loader_conf_files" with the same list.

I first tried to solve it by temporarily removing "loader_conf_files" from the environment before loading any file and then checking if it was set in the meantime. This approach works but has two (arguably minor) problems: each processed file cannot itself on the previous value of "loader_conf_files", and this causes lots of calls to setenv/unsetenv/getenv (don't know if this is really a problem, but some comments made me think it could).

So I settled for another approach, which doesn't have any of these drawbacks. Instead, it depends on the use of "env_changed" (and the fact that parsing a configuration file sets environment variables through "setEnv").

I've replaced readConfFiles by a simple readConf (which still can be passed the "loaded_files" map, but this is not used internally; the argument could be removed), which is used by "config.load" as well as "read-conf" straight.

What do you think?
Comment 7 Kyle Evans freebsd_committer 2020-04-29 12:36:46 UTC
I'll look at the patch in a bit, but my initial impression is that I might have solved these concerns in a follow-up commit. The final value of loader_conf_files is insignificant, so the latest version of read-conf sets it to empty before descending into readConfFiles so that any files to load get picked up.
Comment 8 Olivier Certner 2020-04-29 19:16:06 UTC
(In reply to Kyle Evans from comment #7)

Yes, I overlooked 360423, thinking it was wrong (but only the added comment is ;-)). So you're right, the original problem is fixed.

Still, the proposed patch applies POLA for "loader_conf_files", which is a priori a variable like the others, in addition to its special meaning when loading, by keeping the latest value it was set at in between loads and at the end. I have no use for this myself, and a priori cannot think of useful consequences of that. But still, I think that if "loader_conf_files" can be clobbered by the loading process (the current state), this should be mentioned in the documentation for this variable ("loader_conf_files").

Also, I feel that readConf is a more to the point interface for the existing code, but it may be a matter of taste, and you might have other usage plans that contradict this. Anyway, it's up to you since you maintain this code.

Thanks.
Comment 9 Kyle Evans freebsd_committer 2020-04-29 19:31:05 UTC
(In reply to Olivier Certner from comment #8)

I will amend the documentation to indicate that one shouldn't rely on any particular value of loader_conf_files, it should be considered write-only and this is historically true.

I do like your version, config.readConf, more; I will likely commit this later tonight as it's a much better and more comfortable interface indeed. With the above-mentioned documentation change, this can be simplified a little bit as we shouldn't bother doing any sort of bookkeeping with loader_conf_files.

I may go a step further and add in a means by which we can restrict loader_conf_files (or other arbitrary variables) from even making it into the actual loader environment.
Comment 10 Olivier Certner 2020-04-29 22:10:04 UTC
(In reply to Kyle Evans from comment #9)

OK, perfect!
Comment 11 commit-hook freebsd_committer 2020-04-30 21:04:53 UTC
A commit references this bug:

Author: kevans
Date: Thu Apr 30 21:04:40 UTC 2020
New revision: 360506
URL: https://svnweb.freebsd.org/changeset/base/360506

Log:
  lualoader: config: improve readConfFiles, rename to readConf

  The previous interface was pretty bad, and required the caller to get some
  implementation details correct that it really shouldn't need to (e.g.
  loader_conf_files handling) and pass in an empty table for it to use.

  The new and much improved interface, readConf, is much less of a hack;
  hiding these implementation details and just doing the right thing.
  config.lua will now use it to process /boot/defaults/loader.conf and the
  subsequent loader_conf_files from there, and read-conf will also use it.

  This improvement submitted by Olivier (cited below), loader_conf_files
  handling from the original patch was changed to just clobber it before
  processing and not bother restoring it after the fact following r360505
  where it's now guaranteed to evade the loader environment.

  PR:		244640
  Submitted by:	Olivier Certner (olivier freebsd free fr>

Changes:
  head/stand/lua/cli.lua
  head/stand/lua/config.lua
  head/stand/lua/config.lua.8
Comment 12 Kyle Evans freebsd_committer 2020-04-30 21:29:24 UTC
Committed mostly unchanged, thanks! Do feel free to submit more patches for clean-ups like this; a good chunk of the later lualoader design was made under duress of sorts, and I haven't found time to step back and do an objective analysis on it with a clean view.
Comment 13 Olivier Certner 2020-05-01 12:51:22 UTC
Thanks a lot!

I'll MFC this to 12 tonight or tomorrow, so I'll post a new patch for 12 as well if you do not commit it in the meantime.

No problem, I'll submit other clean-up patches when I have the occasion.
Comment 14 Olivier Certner 2020-05-02 20:12:58 UTC
Created attachment 214036 [details]
MFC to 12

MFC for 12.

In fact, merging was trivial. This is the patch produced applying on 12 the patch produced by `svn diff -r 360419:360506` on head.

Tested the change, default loading or custom loading through `read-conf` work OK. I did not exercise the `loader_conf_files` part in the non-default case.
Comment 15 Kyle Evans freebsd_committer 2020-05-03 03:43:13 UTC
Excellent, thanks! Will be MFC'ing the whole batch here shortly. =-)
Comment 16 commit-hook freebsd_committer 2020-05-03 03:54:33 UTC
A commit references this bug:

Author: kevans
Date: Sun May  3 03:53:40 UTC 2020
New revision: 360596
URL: https://svnweb.freebsd.org/changeset/base/360596

Log:
  MFC lualoader read-conf support: r360420-r360423, r360425, r360427, r360486,
  \r360505-r360506

  r360420:
  lualoader config: don't call loader.getenv() as much

  We don't actually need to fetch loader_conf_files as much as we do; we've
  already fetched it once at the beginning, we only really need to fetch it
  again after each file we've processed. If it changes, then we can stash that
  off into our local prefiles.

  While here, drop a note about the recursion so that I stop trying to
  change it. It may very well make redundant some of the work we're doing, but
  that's OK.

  r360421:
  lualoader: config: start exporting readConfFiles

  In the process, change it slightly: readConfFiles will take a string like
  loader_conf_files in addition to the loaded_files table that it normally
  takes. This is to facilitate the addition of a read-conf CLI command, which
  will just pass in the single file to read and an empty table.

  r360422:
  lualoader: cli: add read-conf

  This is a straightforward match to the command used by many in forthloader;
  it uses the newly-exported config.readConfFiles() to make sure that any
  loader_conf_files gets done as appropriate.

  r360423:
  lualoader: cli: clobber loader_conf_files before proceeding

  This makes sure that config.readConfFiles doesn't see a stale
  loader_conf_files from before, in case the newly loaded file doesn't set it.

  r360425:
  config.lua(8): "may should" is not proper grammar

  r360427:
  config.lua(8): catch up to recently added hooks

  While we're here, let's stylize these as functions instead of just raw text.
  A future change may allow arbitrary data arguments to be passed some of
  these, and the distinction is useful.

  r360486:
  loader.conf(5): document that loader_conf_files may be clobbered

  A future change in lualoader may take some liberties with the
  loader_conf_files in the name of efficiency; namely, it may start omitting
  it from the loader environment entirely so that it doesn't need to worry
  about maintaining any specific value.

  This variable has historically been incredibly volatile anyways, as it may
  get set to completely different values in any given configuration file to
  trigger a load of more files.

  Document now that we may not maintain it in the future, but perhaps we'll
  reserve the right to change our minds and eventually formally export all of
  the loader configuration files that were read using this variable.

  r360505:
  lualoader: config: add a table for restricted environment vars

  This new table should be used for transient values that don't need to end up
  in the loader environment. Generally, these will be things that are internal
  details that really aren't needed or interesting outside of the config
  module (e.g. if we changed how ${module}_* directives work, they might use
  this instead).

  To start, populate it with loader_conf_files. Any specific value of
  loader_conf_files isn't all that interesting; if we're going to export it,
  we should really instead export a loader_conf_files that indicates all of
  the configuration files we processed. This will be used to reduce
  bookkeeping overhead in a future commit that cleans up readConfFiles.

  r360506:
  lualoader: config: improve readConfFiles, rename to readConf

  The previous interface was pretty bad, and required the caller to get some
  implementation details correct that it really shouldn't need to (e.g.
  loader_conf_files handling) and pass in an empty table for it to use.

  The new and much improved interface, readConf, is much less of a hack;
  hiding these implementation details and just doing the right thing.
  config.lua will now use it to process /boot/defaults/loader.conf and the
  subsequent loader_conf_files from there, and read-conf will also use it.

  This improvement submitted by Olivier (cited below), loader_conf_files
  handling from the original patch was changed to just clobber it before
  processing and not bother restoring it after the fact following r360505
  where it's now guaranteed to evade the loader environment.

  PR:		244640

Changes:
_U  stable/11/
  stable/11/stand/defaults/loader.conf.5
  stable/11/stand/lua/cli.lua
  stable/11/stand/lua/config.lua
  stable/11/stand/lua/config.lua.8
_U  stable/12/
  stable/12/stand/defaults/loader.conf.5
  stable/12/stand/lua/cli.lua
  stable/12/stand/lua/config.lua
  stable/12/stand/lua/config.lua.8
Comment 17 Olivier Certner 2020-05-03 22:39:47 UTC
Cool! Thank you! As for me, you can close this PR.