Bug 244640 (lua_loader_read-conf)
| Summary: | [Patch][Lua loader] Implement missing "read-conf" 4th command | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Olivier Certner <olce> | ||||||||
| Component: | misc | Assignee: | Kyle Evans <kevans> | ||||||||
| Status: | Closed FIXED | ||||||||||
| Severity: | Affects Some People | CC: | bugs, kevans | ||||||||
| Priority: | --- | Keywords: | loader, patch-ready | ||||||||
| Version: | CURRENT | Flags: | olce:
mfc-stable12?
olce: mfc-stable11? |
||||||||
| Hardware: | Any | ||||||||||
| OS: | Any | ||||||||||
| Attachments: |
|
||||||||||
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. 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 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. 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.
(In reply to Olivier Certner from comment #4) Sure thing- iteration is good. =-) 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?
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. (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. (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. (In reply to Kyle Evans from comment #9) OK, perfect! 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 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. 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. 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.
Excellent, thanks! Will be MFC'ing the whole batch here shortly. =-) 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 Cool! Thank you! As for me, you can close this PR. |
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.