| Message ID | 20240905124218.703140-2-fe@dev.tdt.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] base-files: move config_generate to preinit | expand |
Small item, these two patches appear largely independent. Numbering is primarily needed if there are dependencies between patches. As such there seem any need to number these. On Thu, Sep 05, 2024 at 02:42:18PM +0200, Florian Eckert wrote: > The 'preinit' script '/lib/preinit/70_initramfs_test' [1] checks whether > the system is running in an 'initramfs'. If this is the case, the loop [2] > in which the function is called is exited via a 'break' call. All further > 'preinit_main' hooks are no longer processed. Therefore, the check whether > we are running in an initramfs is not necessary and are therefore removed. I dislike this wording. Perhaps replace the 3rd sentence with "Further 'preinit_main' hooks are ignored."? The final sentence seemed a caltrop. A better subject line might be "remove duplicate INITRAMFS checks in preinit_main hooks". The main issue is these replicate the test in 70_initramfs_test. Examining this situation, I wonder whether this is really the right way to go. There are 29 files which match [789]* (excluding `70_initramfs_test`). So you found this redundant check in >10% of files. This seems a rather high percentage. Perhaps it might be better to remove the `break` from `70_initramfs_test` and instead require explicitly checking $INITRAMFS ? I'm also concerned about the test being done during *every* boot. This doesn't make sense unless there were many devices which can use both overlay and initramfs as root. Perhaps the after initramfs scripts should be split from preinit hooks? Perhaps the files should have a tag inside them and during build scripts which require initramfs should be conditionally enabled? Perhaps base-files should be split into base-files, base-files-initramfs and base-files-noinitramfs? In other news 18 of the 29 [789]* files are named "79_move_config" inside target/linux. This makes me wonder how similar the job they're doing is. Perhaps these should all merge into a single file in package/base-files. The patch basically looks reasonable, just there are these concerning bits lurking slightly deeper.
Hello Elliott, On 2024-09-06 04:40, Elliott Mitchell wrote: > Small item, these two patches appear largely independent. Numbering is > primarily needed if there are dependencies between patches. As such > there seem any need to number these. You're right sorry, I summarized it because it makes changes in the same corner. > On Thu, Sep 05, 2024 at 02:42:18PM +0200, Florian Eckert wrote: >> The 'preinit' script '/lib/preinit/70_initramfs_test' [1] checks >> whether >> the system is running in an 'initramfs'. If this is the case, the loop >> [2] >> in which the function is called is exited via a 'break' call. All >> further >> 'preinit_main' hooks are no longer processed. Therefore, the check >> whether >> we are running in an initramfs is not necessary and are therefore >> removed. > > I dislike this wording. Perhaps replace the 3rd sentence with "Further > 'preinit_main' hooks are ignored."? The final sentence seemed a > caltrop. > > A better subject line might be "remove duplicate INITRAMFS checks in > preinit_main hooks". The main issue is these replicate the test in > 70_initramfs_test. I'm not a native speaker. Thanks for the hint. I will definitely adjust the wording if we agree on how to proceed now. > Examining this situation, I wonder whether this is really the right way > to go. > > There are 29 files which match [789]* (excluding `70_initramfs_test`). > So you found this redundant check in >10% of files. This seems a > rather > high percentage. Perhaps it might be better to remove the `break` from > `70_initramfs_test` and instead require explicitly checking $INITRAMFS > ? I thought about that too, but decided against it, as I would then have to adapt a lot more, as you mentioned. I have to admit that at the beginning I didn't understand why my script wasn't called in 'preinit_main' until I saw that there was a 'break' in 'boot_run_hook'. I would not have expected that! I would therefore also prefer as you notice the same to delete 'break' and check the INITRAMFS flag. That makes the whole thing more transparent and clear. > I'm also concerned about the test being done during *every* boot. This > doesn't make sense unless there were many devices which can use both > overlay and initramfs as root. I have update the wiki page [1] with currently used preinit script that I have found to get an overview what is going on. I hope it is complete! If I have seen it correctly there are some scripts that need an additional `[ "$INITRAMFS" = "1" ] ||` check if we remove the 'break' in '70_initramfs_test'. This means that all scripts that have a number greater than '70' and hook into the 'preinit_main' need this check. See my change in the wiki `INITRAMFS check 'NO'` > Perhaps the after initramfs scripts should be split from preinit hooks? If I got the whole picture, we already have a hook 'preinit_mount_root' for scripts that are executed if we are not run in an initramfs. Therefore we can move all other scripts to 'preinit_mount_root' and thus drop the INITRAMFS check? So we only need this check for '80_mount_root'. > Perhaps the files should have a tag inside them and during build > scripts > which require initramfs should be conditionally enabled? What exactly do you mean by that? Should the scripts always be installed, but if an initramfs is built, then the hook line is deleted? > Perhaps base-files should be split into base-files, > base-files-initramfs > and base-files-noinitramfs? This would be a big change from my point of view, because in base-files are not only the preinit scripts but also many other things. We would have to maintain this redundantly, if I understand your intention correctly > In other news 18 of the 29 [789]* files are named "79_move_config" > inside > target/linux. This makes me wonder how similar the job they're doing > is. > Perhaps these should all merge into a single file in > package/base-files. This would make sense if the scripts were all the same, but on closer look they are all different and are target/subtarget specific. Therefore, from my point of view, it makes no sense to move them to the base-files. Best regards Florian [1] https://openwrt.org/docs/techref/preinit_mount#overview
On Fri, Sep 06, 2024 at 02:15:22PM +0200, Florian Eckert wrote: > > On 2024-09-06 04:40, Elliott Mitchell wrote: > > > > Examining this situation, I wonder whether this is really the right way > > to go. > > > > There are 29 files which match [789]* (excluding `70_initramfs_test`). > > So you found this redundant check in >10% of files. This seems a rather > > high percentage. Perhaps it might be better to remove the `break` from > > `70_initramfs_test` and instead require explicitly checking $INITRAMFS ? > > I thought about that too, but decided against it, as I would then have to > adapt a lot more, as you mentioned. I have to admit that at the beginning > I didn't understand why my script wasn't called in 'preinit_main' until > I saw that there was a 'break' in 'boot_run_hook'. > > I would not have expected that! > > I would therefore also prefer as you notice the same to delete 'break' and > check the INITRAMFS flag. That makes the whole thing more transparent and > clear. These haven't been through the copy/update cycle of the kernel configuration files, so `git blame` works on them: f43b7934d285 package/base-files/files/lib/preinit/80_mount_root John Crispin <john@openwrt.org> 2013-03-13 11:11:19 a0b7fef0ffe4 package/utils/zyxel-bootconfig/files/95_apply_bootconfig David Bauer <mail@david-bauer.net> 2022-07-20 12:52:06 fe0081eecf43 target/linux/bcm27xx/base-files/lib/preinit/81_set_root_part Álvaro Fernández Rojas <noltari@gmail.com> 2024-03-04 07:28:57 The authors are all members of the project and should therefore be pretty knowledgeable about OpenWRT. If even members of the project are getting it wrong, that seems strong evidence the existing approach doesn't work well. Add us and I think we've got a consensus the `break` really should be removed and checking in every script should be used. Though there are the other alternative approaches below. > > Perhaps the files should have a tag inside them and during build scripts > > which require initramfs should be conditionally enabled? > > What exactly do you mean by that? Should the scripts always be installed, > but if an initramfs is built, then the hook line is deleted? I was thinking something along the lines of having a in-comment tag for scripts which should be omitted from initramfs. Perhaps "# INITRAMFS_OMIT" and "# INITRAMFS_INCLUDE"? When building the initramfs use `grep -l` or `grep -L` to identify files which should (not) be included. > > Perhaps base-files should be split into base-files, base-files-initramfs > > and base-files-noinitramfs? > > This would be a big change from my point of view, because in base-files > are not only the preinit scripts but also many other things. We would > have to maintain this redundantly, if I understand your intention correctly This might be too big of a change for now. This may well be a bad idea right now and in the future. > > In other news 18 of the 29 [789]* files are named "79_move_config" > > inside > > target/linux. This makes me wonder how similar the job they're doing > > is. > > Perhaps these should all merge into a single file in package/base-files. > > This would make sense if the scripts were all the same, but on closer > look they are all different and are target/subtarget specific. Therefore, > from my point of view, it makes no sense to move them to the base-files. Indeed. A quick glance suggested many are pretty similar and I wonder if it would be possible to write a single script which could replace all of them. I suspect most of them started as copies and they've simply been slowly drifting apart.
diff --git a/package/base-files/files/lib/preinit/80_mount_root b/package/base-files/files/lib/preinit/80_mount_root index 940c56c925..d58ffcd3db 100644 --- a/package/base-files/files/lib/preinit/80_mount_root +++ b/package/base-files/files/lib/preinit/80_mount_root @@ -51,4 +51,4 @@ do_mount_root() { } } -[ "$INITRAMFS" = "1" ] || boot_hook_add preinit_main do_mount_root +boot_hook_add preinit_main do_mount_root diff --git a/package/utils/zyxel-bootconfig/files/95_apply_bootconfig b/package/utils/zyxel-bootconfig/files/95_apply_bootconfig index c98bc8fbe2..9abf138e4a 100644 --- a/package/utils/zyxel-bootconfig/files/95_apply_bootconfig +++ b/package/utils/zyxel-bootconfig/files/95_apply_bootconfig @@ -12,4 +12,4 @@ apply_bootconfig() { esac } -[ "$INITRAMFS" = "1" ] || boot_hook_add preinit_main apply_bootconfig +boot_hook_add preinit_main apply_bootconfig diff --git a/target/linux/bcm27xx/base-files/lib/preinit/81_set_root_part b/target/linux/bcm27xx/base-files/lib/preinit/81_set_root_part index a915150213..fd3f98dd3f 100644 --- a/target/linux/bcm27xx/base-files/lib/preinit/81_set_root_part +++ b/target/linux/bcm27xx/base-files/lib/preinit/81_set_root_part @@ -9,4 +9,4 @@ do_set_root_part() { fi } -[ "$INITRAMFS" = "1" ] || boot_hook_add preinit_main do_set_root_part +boot_hook_add preinit_main do_set_root_part
The 'preinit' script '/lib/preinit/70_initramfs_test' [1] checks whether the system is running in an 'initramfs'. If this is the case, the loop [2] in which the function is called is exited via a 'break' call. All further 'preinit_main' hooks are no longer processed. Therefore, the check whether we are running in an initramfs is not necessary and are therefore removed. [1] https://github.com/openwrt/openwrt/blob/master/package/base-files/files/lib/preinit/70_initramfs_test [2] https://github.com/openwrt/openwrt/blob/master/package/base-files/files/lib/functions/preinit.sh#L57 Signed-off-by: Florian Eckert <fe@dev.tdt.de> --- package/base-files/files/lib/preinit/80_mount_root | 2 +- package/utils/zyxel-bootconfig/files/95_apply_bootconfig | 2 +- target/linux/bcm27xx/base-files/lib/preinit/81_set_root_part | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)