init: Stop combining actions
In the past, I had thought it didn't make sense to have multiple
Action classes with identical triggers within ActionManager::actions_,
and opted to instead combine these into a single action. In theory,
it should reduce memory overhead as only one copy of the triggers
needs to be stored.
In practice, this ends up not being a good idea.
Most importantly, given a file with the below three sections in this
same order:
on boot
setprop a b
on boot && property:true=true
setprop c d
on boot
setprop e f
Assuming that property 'true' == 'true', when the `boot` event
happens, the order of the setprop commands will actually be:
setprop a b
setprop e f
setprop c d
instead of the more intuitive order of:
setprop a b
setprop c d
setprop e f
This is a mistake and this CL fixes it. It also documents this order.
Secondly, with a given 'Action' now spanning multiple files, in order
to keep track of which file a command is run from, the 'Command'
itself needs to store this. Ironically to the original intention,
this increases total ram usage. This change now only stores the file
name in each 'Action' instead of each 'Command'. All in all this is a
negligible trade off of ram usage.
Thirdly, this requires a bunch of extra code and assumptions that
don't help anything else. In particular it forces to keep property triggers
sorted for easy comparison, which I'm using an std::map for currently,
but that is not the best data structure to contain them.
Lastly, I added the filename and line number to the 'processing
action' LOG(INFO) message.
Test: Boot bullhead, observe above changes
Test: Boot sailfish, observe no change in boot time
Change-Id: I3fbcac4ee677351314e33012c758145be82346e9
diff --git a/init/README.md b/init/README.md
index 1837868..1eb42e0 100644
--- a/init/README.md
+++ b/init/README.md
@@ -16,9 +16,7 @@
or options belong to the section most recently declared. Commands
or options before the first section are ignored.
-Actions and Services have unique names. If a second Action is defined
-with the same name as an existing one, its commands are appended to
-the commands of the existing action. If a second Service is defined
+Services have unique names. If a second Service is defined
with the same name as an existing one, it is ignored and an error
message is logged.
@@ -31,13 +29,21 @@
/init.rc is the primary .rc file and is loaded by the init executable
at the beginning of its execution. It is responsible for the initial
-set up of the system. It imports /init.${ro.hardware}.rc which is the
-primary vendor supplied .rc file.
+set up of the system.
-During the mount\_all command, the init executable loads all of the
-files contained within the /{system,vendor,odm}/etc/init/ directories.
-These directories are intended for all Actions and Services used after
-file system mounting.
+Devices that mount /system, /vendor through the early mount mechanism
+load all of the files contained within the
+/{system,vendor,odm}/etc/init/ directories immediately after loading
+the primary /init.rc. This is explained in more details in the
+Imports section of this file.
+
+Legacy devices without the early mount mechanism do the following:
+1. /init.rc imports /init.${ro.hardware}.rc which is the primary
+ vendor supplied .rc file.
+2. During the mount\_all command, the init executable loads all of the
+ files contained within the /{system,vendor,odm}/etc/init/ directories.
+ These directories are intended for all Actions and Services used after
+ file system mounting.
One may specify paths in the mount\_all command line to have it import
.rc files at the specified paths instead of the default ones listed above.
@@ -89,7 +95,7 @@
Actions
-------
Actions are named sequences of commands. Actions have a trigger which
-is used to determine when the action should occur. When an event
+is used to determine when the action is executed. When an event
occurs which matches an action's trigger, that action is added to
the tail of a to-be-executed queue (unless it is already on the
queue).
@@ -106,6 +112,34 @@
<command>
<command>
+Actions are added to the queue and executed based on the order that
+the file that contains them was parsed (see the Imports section), then
+sequentially within an individual file.
+
+For example if a file contains:
+
+ on boot
+ setprop a 1
+ setprop b 2
+
+ on boot && property:true=true
+ setprop c 1
+ setprop d 2
+
+ on boot
+ setprop e 1
+ setprop f 2
+
+Then when the `boot` trigger occurs and assuming the property `true`
+equals `true`, then the order of the commands executed will be:
+
+ setprop a 1
+ setprop b 2
+ setprop c 1
+ setprop d 2
+ setprop e 1
+ setprop f 2
+
Services
--------
@@ -458,21 +492,54 @@
Imports
-------
-The import keyword is not a command, but rather its own section and is
-handled immediately after the .rc file that contains it has finished
-being parsed. It takes the below form:
-
`import <path>`
> Parse an init config file, extending the current configuration.
If _path_ is a directory, each file in the directory is parsed as
a config file. It is not recursive, nested directories will
not be parsed.
-There are only two times where the init executable imports .rc files:
+The import keyword is not a command, but rather its own section,
+meaning that it does not happen as part of an Action, but rather,
+imports are handled as a file is being parsed and follow the below logic.
- 1. When it imports /init.rc during initial boot
- 2. When it imports /{system,vendor,odm}/etc/init/ or .rc files at specified
- paths during mount_all
+There are only three times where the init executable imports .rc files:
+
+ 1. When it imports /init.rc or the script indicated by the property
+ `ro.boot.init_rc` during initial boot.
+ 2. When it imports /{system,vendor,odm}/etc/init/ for early mount
+ devices immediately after importing /init.rc.
+ 3. When it imports /{system,vendor,odm}/etc/init/ or .rc files at specified
+ paths during mount_all.
+
+The order that files are imported is a bit complex for legacy reasons
+and to keep backwards compatibility. It is not strictly guaranteed.
+
+The only correct way to guarantee that a command has been run before a
+different command is to either 1) place it in an Action with an
+earlier executed trigger, or 2) place it in an Action with the same
+trigger within the same file at an earlier line.
+
+Nonetheless, the defacto order for early mount devices is:
+1. /init.rc is parsed then recursively each of its imports are
+ parsed.
+2. The contents of /system/etc/init/ are alphabetized and parsed
+ sequentially, with imports happening recursively after each file is
+ parsed.
+3. Step 2 is repeated for /vendor/etc/init then /odm/etc/init
+
+The below pseudocode may explain this more clearly:
+
+ fn Import(file)
+ Parse(file)
+ for (import : file.imports)
+ Import(import)
+
+ Import(/init.rc)
+ Directories = [/system/etc/init, /vendor/etc/init, /odm/etc/init]
+ for (directory : Directories)
+ files = <Alphabetical order of directory's contents>
+ for (file : files)
+ Import(file)
Properties
diff --git a/init/action.cpp b/init/action.cpp
index 347edb0..c128968 100644
--- a/init/action.cpp
+++ b/init/action.cpp
@@ -26,10 +26,8 @@
using android::base::Join;
using android::base::StringPrintf;
-Command::Command(BuiltinFunction f, const std::vector<std::string>& args,
- const std::string& filename, int line)
- : func_(f), args_(args), filename_(filename), line_(line) {
-}
+Command::Command(BuiltinFunction f, const std::vector<std::string>& args, int line)
+ : func_(f), args_(args), line_(line) {}
int Command::InvokeFunc() const {
std::vector<std::string> expanded_args;
@@ -49,21 +47,12 @@
return Join(args_, ' ');
}
-std::string Command::BuildSourceString() const {
- if (!filename_.empty()) {
- return StringPrintf(" (%s:%d)", filename_.c_str(), line_);
- } else {
- return std::string();
- }
-}
-
-Action::Action(bool oneshot) : oneshot_(oneshot) {
-}
+Action::Action(bool oneshot, const std::string& filename, int line)
+ : oneshot_(oneshot), filename_(filename), line_(line) {}
const KeywordMap<BuiltinFunction>* Action::function_map_ = nullptr;
-bool Action::AddCommand(const std::vector<std::string>& args,
- const std::string& filename, int line, std::string* err) {
+bool Action::AddCommand(const std::vector<std::string>& args, int line, std::string* err) {
if (!function_map_) {
*err = "no function map available";
return false;
@@ -79,20 +68,12 @@
return false;
}
- AddCommand(function, args, filename, line);
+ AddCommand(function, args, line);
return true;
}
-void Action::AddCommand(BuiltinFunction f,
- const std::vector<std::string>& args,
- const std::string& filename, int line) {
- commands_.emplace_back(f, args, filename, line);
-}
-
-void Action::CombineAction(const Action& action) {
- for (const auto& c : action.commands_) {
- commands_.emplace_back(c);
- }
+void Action::AddCommand(BuiltinFunction f, const std::vector<std::string>& args, int line) {
+ commands_.emplace_back(f, args, line);
}
std::size_t Action::NumCommands() const {
@@ -122,7 +103,7 @@
android::base::GetMinimumLogSeverity() <= android::base::DEBUG) {
std::string trigger_name = BuildTriggersString();
std::string cmd_str = command.BuildCommandString();
- std::string source = command.BuildSourceString();
+ std::string source = StringPrintf(" (%s:%d)", filename_.c_str(), command.line());
LOG(INFO) << "Command '" << cmd_str << "' action=" << trigger_name << source
<< " returned " << result << " took " << duration_ms << "ms.";
@@ -234,11 +215,6 @@
return event_trigger_.empty() && CheckPropertyTriggers(name, value);
}
-bool Action::TriggersEqual(const Action& other) const {
- return property_triggers_ == other.property_triggers_ &&
- event_trigger_ == other.event_trigger_;
-}
-
std::string Action::BuildTriggersString() const {
std::vector<std::string> triggers;
@@ -306,17 +282,7 @@
}
void ActionManager::AddAction(std::unique_ptr<Action> action) {
- auto old_action_it =
- std::find_if(actions_.begin(), actions_.end(),
- [&action] (std::unique_ptr<Action>& a) {
- return action->TriggersEqual(*a);
- });
-
- if (old_action_it != actions_.end()) {
- (*old_action_it)->CombineAction(*action);
- } else {
- actions_.emplace_back(std::move(action));
- }
+ actions_.emplace_back(std::move(action));
}
void ActionManager::QueueEventTrigger(const std::string& trigger) {
@@ -332,16 +298,15 @@
QueuePropertyTrigger("", "");
}
-void ActionManager::QueueBuiltinAction(BuiltinFunction func,
- const std::string& name) {
- auto action = std::make_unique<Action>(true);
+void ActionManager::QueueBuiltinAction(BuiltinFunction func, const std::string& name) {
+ auto action = std::make_unique<Action>(true, "<Builtin Action>", 0);
std::vector<std::string> name_vector{name};
if (!action->InitSingleTrigger(name)) {
return;
}
- action->AddCommand(func, name_vector);
+ action->AddCommand(func, name_vector, 0);
trigger_queue_.push(std::make_unique<BuiltinTrigger>(action.get()));
actions_.emplace_back(std::move(action));
@@ -366,7 +331,8 @@
if (current_command_ == 0) {
std::string trigger_name = action->BuildTriggersString();
- LOG(INFO) << "processing action (" << trigger_name << ")";
+ LOG(INFO) << "processing action (" << trigger_name << ") from (" << action->filename()
+ << ":" << action->line() << ")";
}
action->ExecuteOneCommand(current_command_);
@@ -397,15 +363,15 @@
}
}
-bool ActionParser::ParseSection(const std::vector<std::string>& args,
- std::string* err) {
+bool ActionParser::ParseSection(const std::vector<std::string>& args, const std::string& filename,
+ int line, std::string* err) {
std::vector<std::string> triggers(args.begin() + 1, args.end());
if (triggers.size() < 1) {
*err = "actions must have a trigger";
return false;
}
- auto action = std::make_unique<Action>(false);
+ auto action = std::make_unique<Action>(false, filename, line);
if (!action->InitTriggers(triggers, err)) {
return false;
}
@@ -414,10 +380,9 @@
return true;
}
-bool ActionParser::ParseLineSection(const std::vector<std::string>& args,
- const std::string& filename, int line,
- std::string* err) const {
- return action_ ? action_->AddCommand(args, filename, line, err) : false;
+bool ActionParser::ParseLineSection(const std::vector<std::string>& args, int line,
+ std::string* err) {
+ return action_ ? action_->AddCommand(args, line, err) : false;
}
void ActionParser::EndSection() {
diff --git a/init/action.h b/init/action.h
index 0bae9f0..25e5a3e 100644
--- a/init/action.h
+++ b/init/action.h
@@ -27,31 +27,26 @@
#include "keyword_map.h"
class Command {
-public:
- Command(BuiltinFunction f, const std::vector<std::string>& args,
- const std::string& filename, int line);
+ public:
+ Command(BuiltinFunction f, const std::vector<std::string>& args, int line);
int InvokeFunc() const;
std::string BuildCommandString() const;
- std::string BuildSourceString() const;
-private:
+ int line() const { return line_; }
+
+ private:
BuiltinFunction func_;
std::vector<std::string> args_;
- std::string filename_;
int line_;
};
class Action {
-public:
- explicit Action(bool oneshot = false);
+ public:
+ explicit Action(bool oneshot, const std::string& filename, int line);
- bool AddCommand(const std::vector<std::string>& args,
- const std::string& filename, int line, std::string* err);
- void AddCommand(BuiltinFunction f,
- const std::vector<std::string>& args,
- const std::string& filename = "", int line = 0);
- void CombineAction(const Action& action);
+ bool AddCommand(const std::vector<std::string>& args, int line, std::string* err);
+ void AddCommand(BuiltinFunction f, const std::vector<std::string>& args, int line);
bool InitTriggers(const std::vector<std::string>& args, std::string* err);
bool InitSingleTrigger(const std::string& trigger);
std::size_t NumCommands() const;
@@ -60,11 +55,12 @@
bool CheckEventTrigger(const std::string& trigger) const;
bool CheckPropertyTrigger(const std::string& name,
const std::string& value) const;
- bool TriggersEqual(const Action& other) const;
std::string BuildTriggersString() const;
void DumpState() const;
bool oneshot() const { return oneshot_; }
+ const std::string& filename() const { return filename_; }
+ int line() const { return line_; }
static void set_function_map(const KeywordMap<BuiltinFunction>* function_map) {
function_map_ = function_map;
}
@@ -80,6 +76,8 @@
std::string event_trigger_;
std::vector<Command> commands_;
bool oneshot_;
+ std::string filename_;
+ int line_;
static const KeywordMap<BuiltinFunction>* function_map_;
};
@@ -115,18 +113,17 @@
};
class ActionParser : public SectionParser {
-public:
+ public:
ActionParser() : action_(nullptr) {
}
- bool ParseSection(const std::vector<std::string>& args,
+ bool ParseSection(const std::vector<std::string>& args, const std::string& filename, int line,
std::string* err) override;
- bool ParseLineSection(const std::vector<std::string>& args,
- const std::string& filename, int line,
- std::string* err) const override;
+ bool ParseLineSection(const std::vector<std::string>& args, int line, std::string* err) override;
void EndSection() override;
void EndFile(const std::string&) override {
}
-private:
+
+ private:
std::unique_ptr<Action> action_;
};
diff --git a/init/import_parser.cpp b/init/import_parser.cpp
index 8a2bcc2..f66b2ba 100644
--- a/init/import_parser.cpp
+++ b/init/import_parser.cpp
@@ -20,8 +20,8 @@
#include "util.h"
-bool ImportParser::ParseSection(const std::vector<std::string>& args,
- std::string* err) {
+bool ImportParser::ParseSection(const std::vector<std::string>& args, const std::string& filename,
+ int line, std::string* err) {
if (args.size() != 2) {
*err = "single argument needed for import\n";
return false;
diff --git a/init/import_parser.h b/init/import_parser.h
index 0e91025..e15d555 100644
--- a/init/import_parser.h
+++ b/init/import_parser.h
@@ -23,20 +23,20 @@
#include <vector>
class ImportParser : public SectionParser {
-public:
+ public:
ImportParser() {
}
- bool ParseSection(const std::vector<std::string>& args,
+ bool ParseSection(const std::vector<std::string>& args, const std::string& filename, int line,
std::string* err) override;
- bool ParseLineSection(const std::vector<std::string>& args,
- const std::string& filename, int line,
- std::string* err) const override {
+ bool ParseLineSection(const std::vector<std::string>& args, int line,
+ std::string* err) override {
return true;
}
void EndSection() override {
}
void EndFile(const std::string& filename) override;
-private:
+
+ private:
std::vector<std::string> imports_;
};
diff --git a/init/init_parser.cpp b/init/init_parser.cpp
index 53e670b..b425497 100644
--- a/init/init_parser.cpp
+++ b/init/init_parser.cpp
@@ -71,14 +71,13 @@
}
section_parser = section_parsers_[args[0]].get();
std::string ret_err;
- if (!section_parser->ParseSection(args, &ret_err)) {
+ if (!section_parser->ParseSection(args, state.filename, state.line, &ret_err)) {
parse_error(&state, "%s\n", ret_err.c_str());
section_parser = nullptr;
}
} else if (section_parser) {
std::string ret_err;
- if (!section_parser->ParseLineSection(args, state.filename,
- state.line, &ret_err)) {
+ if (!section_parser->ParseLineSection(args, state.line, &ret_err)) {
parse_error(&state, "%s\n", ret_err.c_str());
}
}
diff --git a/init/init_parser.h b/init/init_parser.h
index 6935fdf..64f0cac 100644
--- a/init/init_parser.h
+++ b/init/init_parser.h
@@ -26,11 +26,10 @@
public:
virtual ~SectionParser() {
}
- virtual bool ParseSection(const std::vector<std::string>& args,
- std::string* err) = 0;
- virtual bool ParseLineSection(const std::vector<std::string>& args,
- const std::string& filename, int line,
- std::string* err) const = 0;
+ virtual bool ParseSection(const std::vector<std::string>& args, const std::string& filename,
+ int line, std::string* err) = 0;
+ virtual bool ParseLineSection(const std::vector<std::string>& args, int line,
+ std::string* err) = 0;
virtual void EndSection() = 0;
virtual void EndFile(const std::string& filename) = 0;
};
diff --git a/init/service.cpp b/init/service.cpp
index 4adbbf0..caf5785 100644
--- a/init/service.cpp
+++ b/init/service.cpp
@@ -157,6 +157,7 @@
gid_(0),
namespace_flags_(0),
seclabel_(""),
+ onrestart_(false, "<Service '" + name + "' onrestart>", 0),
ioprio_class_(IoSchedClass_NONE),
ioprio_pri_(0),
priority_(0),
@@ -180,6 +181,7 @@
capabilities_(capabilities),
namespace_flags_(namespace_flags),
seclabel_(seclabel),
+ onrestart_(false, "<Service '" + name + "' onrestart>", 0),
ioprio_class_(IoSchedClass_NONE),
ioprio_pri_(0),
priority_(0),
@@ -438,7 +440,8 @@
bool Service::ParseOnrestart(const std::vector<std::string>& args, std::string* err) {
std::vector<std::string> str_args(args.begin() + 1, args.end());
- onrestart_.AddCommand(str_args, "", 0, err);
+ int line = onrestart_.NumCommands() + 1;
+ onrestart_.AddCommand(str_args, line, err);
return true;
}
@@ -1092,8 +1095,8 @@
}
}
-bool ServiceParser::ParseSection(const std::vector<std::string>& args,
- std::string* err) {
+bool ServiceParser::ParseSection(const std::vector<std::string>& args, const std::string& filename,
+ int line, std::string* err) {
if (args.size() < 3) {
*err = "services must have a name and a program";
return false;
@@ -1110,9 +1113,8 @@
return true;
}
-bool ServiceParser::ParseLineSection(const std::vector<std::string>& args,
- const std::string& filename, int line,
- std::string* err) const {
+bool ServiceParser::ParseLineSection(const std::vector<std::string>& args, int line,
+ std::string* err) {
return service_ ? service_->ParseLine(args, err) : false;
}
diff --git a/init/service.h b/init/service.h
index 5e89b9f..6fe0258 100644
--- a/init/service.h
+++ b/init/service.h
@@ -212,18 +212,15 @@
};
class ServiceParser : public SectionParser {
-public:
- ServiceParser() : service_(nullptr) {
- }
- bool ParseSection(const std::vector<std::string>& args,
+ public:
+ ServiceParser() : service_(nullptr) {}
+ bool ParseSection(const std::vector<std::string>& args, const std::string& filename, int line,
std::string* err) override;
- bool ParseLineSection(const std::vector<std::string>& args,
- const std::string& filename, int line,
- std::string* err) const override;
+ bool ParseLineSection(const std::vector<std::string>& args, int line, std::string* err) override;
void EndSection() override;
- void EndFile(const std::string&) override {
- }
-private:
+ void EndFile(const std::string&) override {}
+
+ private:
bool IsValidName(const std::string& name) const;
std::unique_ptr<Service> service_;