[PATCH 2/2] treewide: remove INITRAMFS check for preinit_main hook

[Florian Eckert]

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