Add system init #3192
Add system init #3192
Conversation
There was a problem hiding this comment.
The testing looks really thorough, great job! This is a very large PR though, with 7 commits. Is there any way it can broken up? Not sure if @jerpelea would find that easier when generating release notes, or if that only applies to the NuttX kernel. Otherwise looks good from me.
@linguini1 Yes, this PR is quite large because it includes a newly added feature. On the premise of ensuring complete functionality, I have split the commits as much as possible, but the review of the first commit - "system/init: Add system init" - will still take a lot of effort. Fortunately, the review of the remaining commits will be much easier. |
There was a problem hiding this comment.
nice work @JianyuWang0623 !!!
Please add an Documentation about this system init. Although a documentation for Android exists, NuttX will need a Documentation/ with specific case uses and configurations
|
Thank you @JianyuWang0623 looks really nice :-) I am a bit afraid that "system" and "init" are too broad terms and may cause confusion.. maybe it would be better to call them "Android System Init" and name config variables something like |
+1 :-) |
|
@xiaoxiang781216 some months ago you commented about planing to add support to SystemV Init. Is this Android Init a replacement or do you still planing to add support to SystemV Init? |
@cederom The configuration is named |
@acassis @cederom Thanks! Good call—we definitely need to add the docs. Could you please review the pull request? apache/nuttx#17215 |
we decide to use Andriod init style since it's more simpler and clear. BTW, the copyright used by Andriod is same as NuttX. |
Thanks @xiaoxiang781216 then I would advise to use "Android System Init" naming conventions in the all identifiers so things are self-explanatory and people know at first glance which system init standard is used? What do you think guys ? :-) |
cederom
requested review from
hartmannathan,
jerpelea,
lupyuen,
raiden00pl and
simbit18
There was a problem hiding this comment.
Thank you @JianyuWang0623 amazing work :-)
I would advise using "Android System Init" naming convention as there are several different init types out there and the name / variables would be self-explanatory and maybe one day add other init types.
But if others are fine with this assumption that "system init" is "android system init" then I will follow :-)
| list_for_every_entry_continue(am->current, &am->actions, | ||
| struct action_s, node) | ||
| { | ||
| if (am->current->event && !strcmp(am->current->event, |
There was a problem hiding this comment.
Please consider using strncmp() instead to avoid stack overflow
There was a problem hiding this comment.
@acassis Thank you for the reminder. We have also considered this issue. However, the good news is that the values of both am->current->event and am->events[i] are obtained from the strdup() function, which normally includes a terminating null byte.
There was a problem hiding this comment.
strncmp with a max length can still protect from bogus user input. I recommend doing something here. the boot process is critical and a user should not be able to mess things up with bogus data injected in the filesystem.
There was a problem hiding this comment.
Just a feedback after dev@ mailing list discussion, thank you for understanding / considering @JianyuWang0623 :
- We clearly need to use "Android System Init" naming here and update all kconfig/global variables names to clearly distinguish from current NuttX Init, Android System Init, and SystemV Init (there is sill a change it will show up here one day), maybe others. "system_system" or "system_init" or "init_main" is really confusing and not self-explanatory, does not even point to "android_system_init". I know this is a bit longer but will provide important context and clear separation between completely different functionalities / implementations.
- We need better documentation for this "Android System Init" as well as "Fastboot". These functionalities are welcome, people want to use it, but users / developers ask questions like: Why do we need another init system? What should happen with the previous ones? What does this new init system solve? How this relates to Fastboot? When, why, and how can I use it? What are best use cases with some examples? Please consider approach like explaining and convincing someone who wants to use it but has zero knowledge about the solution.
@cederom The following is part of the reply.
As I supplemented in the data for the PR apache/nuttx#17216 , NSH, when used as an initialization program, may seem relatively bloated in scenarios where code size is a major concern. NSH and Init can coexist, allowing users to choose based on their own needs. Init addresses issues such as the excessive code size of NSH when used as an initialization program, and its lack of management (e.g., restarting) for daemons.
For the ELF file, Init requires 61,488 bytes, while NSH and NSH_LIBRARY together require 617,632 bytes. Switching from NSH to Init can save a significant amount of space(556,144 bytes).
Init and Fastboot have nothing to do with each other; it's just that a daemon is needed as an example of a service.
Here are some simple examples: When code size is a major concern and no shell is needed, replacing NSH with Init can save a significant amount of space; management functions such as restarting are required when a daemon fails to start or exits abnormally; and so on.
An example has already been added to apache/nuttx#17215. Some supplementary explanations:
|
| FAR struct action_cmd_s *running; | ||
| int pid_running; | ||
| #if defined(CONFIG_SYSTEM_INIT_ACTION_WARN_SLOW) && \ | ||
| CONFIG_SYSTEM_INIT_ACTION_WARN_SLOW > 0 |
There was a problem hiding this comment.
We are always inline-ing the CONFIG_SYSTEM_INIT_ACTION_WARN_SLOW > 0 in every check in the c files.
Would it not help to assert that here, in the header file? something like
#if defined(CONFIG_SYSTEM_INIT_ACTION_WARN_SLOW) && \
CONFIG_SYSTEM_INIT_ACTION_WARN_SLOW > 0
# error Threshold should be positive
#endif
Any other piece of code we can only use #if defined(CONFIG_SYSTEM_INIT_ACTION_WARN_SLOW)
There was a problem hiding this comment.
@tinnedkarma A value of zero is not an exception; rather, like being undefined, it indicates that this function is disabled.
SYSTEM_INIT_ACTION_WARN_SLOW
| Value | The function |
|---|---|
| Undefined | Disable |
| Zero | Disable |
| Greater than zero | Enable |
There was a problem hiding this comment.
I think I've made a mistake by copying the lines.
There should be CONFIG_SYSTEM_INIT_ACTION_WARN_SLOW < 0.
In my example above, I forgot to change the sign from ">" to "<".
Anyway, maybe I did not understand correctly, we need to make sure the option have a positive integer for the value, right?
| } | ||
|
|
||
| #ifdef CONFIG_SYSTEM_INIT_DEBUG | ||
| void init_dump_actions(FAR struct list_node *head) |
There was a problem hiding this comment.
Can't this be moved directly to init.c?
It's only used in init.c, and it does not depend on anything from action.c specifically
- head is passed from init.c
- action is defined inside function scope
- struct action_s is the literal type
- node is the literal member
Nothing from the action.c is referenced here. Am I wrong?
There was a problem hiding this comment.
@tinnedkarma You're right, but from a functional standpoint, this is part of the action.
| * Private Functions | ||
| ****************************************************************************/ | ||
|
|
||
| static int cmd_class_start(FAR struct action_manager_s *am, |
There was a problem hiding this comment.
This function just calls another function.
I suggest making it static inline
There was a problem hiding this comment.
@tinnedkarma I'm not sure if you've read the entire file.
static const struct cmd_map_s g_builtin[] =
{
{"class_start", 2, 2, cmd_class_start},
{"class_stop", 2, 2, cmd_class_stop},
{"exec", 3, 99, cmd_exec},
{"exec_start", 2, 2, cmd_exec_start},
{"start", 2, 2, cmd_start},
{"stop", 2, 2, cmd_stop},
{"trigger", 2, 2, cmd_trigger},
};There was a problem hiding this comment.
Oh, sorry, I may have mislead you with my previous suggestion.
I suggested to also add inline to the function signature, not drop the return value.
A function call creates a new stack, so here we have a stack for the cmd_class_start, and immediately after we create a new stack for init_service_start_by_class. By inline-ing the cmd_class_start we can avoid creating a useless stack.
Correct me if I am wrong.
There was a problem hiding this comment.
@tinnedkarma The function is only used for function pointers. What I'm wondering is whether 'inline' has any practical significance?
There was a problem hiding this comment.
@JianyuWang0623 To my knowledge yes, It's also my point, there is no "internal" logic to this function, so we could avoid incrementing the stack depth with another push-ret if it does not offer any practical significance.
Anyway, maybe another opinion will help.
There was a problem hiding this comment.
this function is called through pointer, inline can't help here anyway.
| if (ret != 0) | ||
| { | ||
| #ifdef CONFIG_SYSTEM_SYSTEM | ||
| char cmd[CONFIG_SYSTEM_INIT_RC_LINE_MAX]; |
There was a problem hiding this comment.
This is also a pointer, should we add FAR?
There was a problem hiding this comment.
char array doesn't add FAR since it allocate in stack directly
| #define init_err(...) | ||
| #endif | ||
|
|
||
| #define init_log(p, ...) \ |
There was a problem hiding this comment.
Any reason why this should not be an switch case?
There was a problem hiding this comment.
@tinnedkarma Anything wrong? Are you sure that converting it to a switch-case is feasible or better?
There was a problem hiding this comment.
Here is my reasoning:
- a do while(0) loop is used, but the loop wont ever "loop", so it's a do? :D Then is the same as using naked {}, which will create a scope and execute the instructions once.
- there is a set/fixed amount of log levels defined in the syslog.h, so it's arguably better to uses a switch case (look-up table) than "falling through" if-else.
I do see the same concept used throughout, so maybe there are other reasons that I may not know, hence why I addressed this as a question.
There was a problem hiding this comment.
a do while(0) loop is used, but the loop wont ever "loop", so it's a do? :D Then is the same as using naked {}, which will create a scope and execute the instructions once.
Common usage; it can avoid syntax issues such as those occurring after expansion:
#define LOG_ERROR(msg) \
fprintf(stderr, "Error: %s\n", msg); \
exit(1);
if (error)
LOG_ERROR(...);
/* After preprocessing */
if (error)
fprintf(...); exit(1);As for runtime efficiency, the compiler will perform optimizations.
there is a set/fixed amount of log levels defined in the syslog.h, so it's arguably better to uses a switch case (look-up table) than "falling through" if-else.
This requires listing all levels; moreover, some code inspection tools will consider that some cases are missing breaks.
switch (p)
{
case LOG_EMERG:
case LOG_ALERT:
case LOG_CRIT:
case LOG_ERR:
init_err(__VA_ARGS__);
break;
case LOG_WARNING:
init_warn(__VA_ARGS__);
break;
/* ... ... */
}include/syslog.h:
#define LOG_EMERG 0 /* System is unusable */
#define LOG_ALERT 1 /* Action must be taken immediately */
#define LOG_CRIT 2 /* Critical conditions */
#define LOG_ERR 3 /* Error conditions */
#define LOG_WARNING 4 /* Warning conditions */
#define LOG_NOTICE 5 /* Normal, but significant, condition */
#define LOG_INFO 6 /* Informational message */
#define LOG_DEBUG 7 /* Debug-level message */There was a problem hiding this comment.
Common usage; it can avoid syntax issues such as those occurring after expansion:
Yes but the same issues can be avoided by adding the curly braces.
As for runtime efficiency, the compiler will perform optimizations.
No, the compiler will not optimize the code in any way, is will generate predicable assembly.
Here is an example of code optimization apache/nuttx#17206.
Personally I would avoid intentionally offloading work to the compiler. It's our job to write predictable code.
This requires listing all levels; moreover, some code inspection tools will consider that some cases are missing breaks.
Yet, I still think it's a cleaner. :D
We are not getting anywhere. That is my personal view, it's fine if we do not agree, and anyway more input is always welcome.
There was a problem hiding this comment.
The do-while pattern is actually quite common, there are some scenarios where regular curly braces don't have the same semantics. That's why do-while (0) is used almost always for multiline macros: https://stackoverflow.com/questions/1067226/c-multi-line-macro-do-while0-vs-scope-block
It does get compiler-optimized out and I think it's clear enough to keep instead of curly braces.
All this information information would be nice have in the documentation :-)
I see, then there is nothing blocking the road to name it "Android System Init" right? We could place it in Thanks again @JianyuWang0623 :-) |
@cederom I still have some questions.
Thank you :-) |
Here I may also add to the third bullet. As I also asked this in the mailing list.
I've saw some talk about SysV type init, so I am not sure what should be the outcome, this is the reason I also asked. |
Refer to the implementation of Android Init Language, consists of five broad
classes of statements: Actions, Commands, Services, Options, and Imports.
Actions support two types of triggers: event and action. Action triggers also
support runtime triggering. Services support lifecycle management, including
automatic restart (at specified intervals), and starting/stopping
individually or by class. Import supports files or directories, and we may
add a static method in the future. The following are some differences:
1. The Android Init Language treats lines starting with `#` as comments,
while we use a preprocessor to handle comments.
2. For action commands, we can omit "exec" and directly execute
built-in apps or nsh builtins.
3. Regarding the property service, users can either adapt it by self or
directly use the preset NVS-based properties.
4. Only part of standard action commands and service options are
implemented currlently.
To enable system/init:
```diff
-CONFIG_INIT_ENTRYPOINT="nsh_main"
+CONFIG_INIT_ENTRYPOINT="init_main"
+CONFIG_SYSTEM_INIT=y
```
For format and additional details, refer to:
https://android.googlesource.com/platform/system/core/+/
master/init/README.md
Signed-off-by: wangjianyu3 <[email protected]>
Usage: class_start <classname> class_stop <classname> Signed-off-by: wangjianyu3 <[email protected]>
Enable CONFIG_SYSTEM_SYSTEM to support nsh builtins, which depends on nsh. Signed-off-by: wangjianyu3 <[email protected]>
If the command of an action takes too long (greater than
CONFIG_SYSTEM_INIT_ACTION_WARN_SLOW milliseconds, 50 ms by default),
a warning log will be output for analysis and debugging.
For example:
a. sleep 1
[ 0.340000] [ 3] [ 0] init_main: executing NSH command 'sleep 1'
[ 1.360000] [ 3] [ 0] init_main: NSH command 'sleep 1' exited 0
> [ 1.360000] [ 3] [ 0] init_main: command 'sleep' took 1020 ms
b. hello (add sleep(1) to examples/hello)
[ 1.390000] [ 3] [ 0] init_main: executed command 'hello' pid 14
[ 1.390000] [ 3] [ 0] init_main: waiting 'hello' pid 14
Hello, World!!
> [ 2.400000] [ 3] [ 0] init_main: command 'hello' pid 14 took 1010 ms
[ 2.400000] [ 3] [ 0] init_main: command 'hello' pid 14 exited status 0
Signed-off-by: wangjianyu3 <[email protected]>
Add support for the oneshot option to the service.
Test
- RC
service telnet telnetd
class test
> oneshot
restart_period 3000
- Runtime
[ 0.150000] [ 3] [ 0] init_main: == Dump Services ==
...
[ 0.160000] [ 3] [ 0] init_main: Service 0x40486aa8 name 'telnet' path 'telnetd'
[ 0.160000] [ 3] [ 0] init_main: pid: 0
[ 0.160000] [ 3] [ 0] init_main: arguments:
[ 0.160000] [ 3] [ 0] init_main: [0] 'service'
[ 0.160000] [ 3] [ 0] init_main: [1] 'telnet'
[ 0.160000] [ 3] [ 0] init_main: [2] 'telnetd'
[ 0.160000] [ 3] [ 0] init_main: classes:
[ 0.160000] [ 3] [ 0] init_main: 'test'
[ 0.170000] [ 3] [ 0] init_main: restart_period: 3000
[ 0.170000] [ 3] [ 0] init_main: reboot_on_failure: -1
[ 0.170000] [ 3] [ 0] init_main: flags:
> [ 0.170000] [ 3] [ 0] init_main: 'oneshot'
...
[ 0.370000] [ 3] [ 0] init_main: starting service 'telnet' ...
[ 0.380000] [ 3] [ 0] init_main: service 'telnet' flag 0x2 add 0x4
[ 0.380000] [ 3] [ 0] init_main: +flag 'running'
[ 0.380000] [ 3] [ 0] init_main: service 'telnet' flag 0x6 add 0x8
[ 0.380000] [ 3] [ 0] init_main: -flag 'restarting'
[ 0.380000] [ 3] [ 0] init_main: service 'telnet' flag 0x6 add 0x1
[ 0.380000] [ 3] [ 0] init_main: -flag 'disabled'
[ 0.380000] [ 3] [ 0] init_main: started service 'telnet' pid 9
...
nsh> kill -9 9
nsh> [ 7.350000] [ 3] [ 0] init_main: service 'telnet' flag 0x6 add 0x4
[ 7.350000] [ 3] [ 0] init_main: -flag 'running'
[ 7.350000] [ 3] [ 0] init_main: service 'telnet' flag 0x2 add 0x80000001
[ 7.350000] [ 3] [ 0] init_main: +flag 'disabled'
[ 7.350000] [ 3] [ 0] init_main: +flag 'remove'
[ 7.350000] [ 3] [ 0] init_main: service 'telnet' pid 9 exited status 1
> [ 7.360000] [ 3] [ 0] init_main: removing service 'telnet' ...
Signed-off-by: wangjianyu3 <[email protected]>
Format: exec_start <service> Start the specified service and pause the processing of any additional initialization commands until the service completes its execution. This command operates similarly to the `exec` command; the key difference is that it utilizes an existing service definition rather than requiring the `exec` argument vector. This feature is particularly intended for use with the `reboot_on_failure` built-in command to perform all types of essential checks during system boot. Signed-off-by: wangjianyu3 <[email protected]>
Add support for the reboot_on_failure option to the service.
When the execution of a command within a certain action fails (returning
a non-zero status code), init will continue to execute subsequent commands or
actions and will not proactively terminate the startup process. To implement
the functionality of "terminating the startup process after a command
execution fails", there are two methods:
a. Execute conditional statements (if/then/else/fi) via exec command,
but this depends on sh:
```
on init
exec -- set +e; \
mount -t fatfs /dev/data /data ; \
if [ $? -ne 0 ] ; \
then \
echo "failed" ; \
reboot ; \
else \
echo "succeed" ; \
fi;
```
b. Via service's oneshot + reboot_on_failure:
/* Although the example uses sh, it does not depend on it and can be
* replaced with any other Builtin Apps.
*/
```
on init
exec_start mkdir_tmp
ls /tmp
service mkdir_tmp sh -c "mkdir /tmp"
reboot_on_failure 0
oneshot
```
Test
- RC
service console sh
class core
override
> reboot_on_failure 0
restart_period 10000
- Runtime
[ 0.150000] [ 3] [ 0] init_main: == Dump Services ==
...
[ 0.170000] [ 3] [ 0] init_main: Service 0x40486ea0 name 'console' path 'sh'
[ 0.170000] [ 3] [ 0] init_main: pid: 0
[ 0.170000] [ 3] [ 0] init_main: arguments:
[ 0.170000] [ 3] [ 0] init_main: [0] 'service'
[ 0.170000] [ 3] [ 0] init_main: [1] 'console'
[ 0.170000] [ 3] [ 0] init_main: [2] 'sh'
[ 0.170000] [ 3] [ 0] init_main: classes:
[ 0.170000] [ 3] [ 0] init_main: 'core'
[ 0.170000] [ 3] [ 0] init_main: restart_period: 10000
> [ 0.170000] [ 3] [ 0] init_main: reboot_on_failure: 0
[ 0.170000] [ 3] [ 0] init_main: flags:
[ 0.170000] [ 3] [ 0] init_main: 'override'
...
[ 0.380000] [ 3] [ 0] init_main: started service 'console' pid 4
...
nsh> kill -9 4
[ 8.060000] [ 3] [ 0] init_main: service 'console' flag 0x20000004 add 0x4
[ 8.060000] [ 3] [ 0] init_main: -flag 'running'
[ 8.060000] [ 3] [ 0] init_main: service 'console' flag 0x20000000 add 0x8
[ 8.060000] [ 3] [ 0] init_main: +flag 'restarting'
> [ 8.060000] [ 3] [ 0] init_main: Error reboot on failure of service 'console' reason 0
Signed-off-by: wangjianyu3 <[email protected]>
JianyuWang0623
dismissed stale reviews from xiaoxiang781216 and linguini1
via
a293013
JianyuWang0623
force-pushed
the
system_init
branch
from
8c8645f to
a293013
Compare
JianyuWang0623
requested review from
acassis,
hartmannathan,
linguini1,
tinnedkarma and
xiaoxiang781216
| #define TIMESPEC2MS(t) (((t).tv_sec * 1000) + (t).tv_nsec / 1000000) | ||
|
|
||
| #ifdef CONFIG_SYSTEM_INIT_DEBUG | ||
| #define init_debug(...) syslog(LOG_DEBUG, ##__VA_ARGS__) |
There was a problem hiding this comment.
I would add the user space mask, LOG_DEBUG | LOG_USER. Not sure if there's a better subsystem mask to use but I don't think kernel is correct here.
|
|
||
| struct service_class_s | ||
| { | ||
| struct list_node node; |
There was a problem hiding this comment.
for consistency you should document the expected structs everywhere.
|
|
||
| if (check_flags(service, SVC_GENTLE_KILL)) | ||
| { | ||
| init_debug("Gentel kill"); |
|
|
||
| if (create) | ||
| { | ||
| s = calloc(1, sizeof(*s)); |
There was a problem hiding this comment.
whats the best practice?
- sizeof of target variable?
- sizeof of intended type?
8 participants
Summary
Related PR:
Add system init
Refer to the implementation of Android Init Language, consists of five broad classes of statements: Actions, Commands, Services, Options, and Imports.
Actions support two types of triggers: event and action. Action triggers also support runtime triggering. Services support lifecycle management, including automatic restart (at specified intervals), and starting/stopping individually or by class. Import supports files or directories, and we may add a static method in the future. The following are some differences:
#as comments, while we use a preprocessor to handle comments.To enable system/init:
For format and additional details, refer to: https://android.googlesource.com/platform/system/core/+/master/init/README.md
Add class start/stop for service
Usage:
Add NSH builtin support for action
Enable
CONFIG_SYSTEM_SYSTEMto support nsh builtins, which depends on nsh.Warning for long commands
If the command of an action takes too long (greater than
CONFIG_SYSTEM_INIT_ACTION_WARN_SLOWmilliseconds, 50 ms by default), a warning log will be output for analysis and debugging.For example:
Add oneshot support for service
Add support for the oneshot option to the service.
Test
Add exec_start support for action
Format:
exec_start <service>Start the specified service and pause the processing of any additional initialization commands until the service completes its execution. This command operates similarly to the
execcommand; the key difference is that it utilizes an existing service definition rather than requiring theexecargument vector.This feature is particularly intended for use with the
reboot_on_failurebuilt-in command to perform all types of essential checks during system boot.Add reboot_on_failure for service
Add support for the reboot_on_failure option to the service.
When the execution of a command within a certain action fails (returning a non-zero status code), init will continue to execute subsequent commands or actions and will not proactively terminate the startup process. To implement the functionality of "terminating the startup process after a command execution fails", there are two methods:
Although the example uses sh, it does not depend on it and can be replaced with any other Builtin Apps.
Test
Impact
Testing