From 012c573e267b8dd70de14237cb470bd7301ee8ea Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Tue, 18 Apr 2017 13:21:54 -0700 Subject: [PATCH] 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 --- init/README.md | 103 ++++++++++++++++++++++++++++++++++------- init/action.cpp | 77 +++++++++--------------------- init/action.h | 39 +++++++--------- init/import_parser.cpp | 4 +- init/import_parser.h | 12 ++--- init/init_parser.cpp | 5 +- init/init_parser.h | 9 ++-- init/service.cpp | 14 +++--- init/service.h | 17 +++---- 9 files changed, 153 insertions(+), 127 deletions(-) diff --git a/init/README.md b/init/README.md index 1837868c7..1eb42e00b 100644 --- a/init/README.md +++ b/init/README.md @@ -16,9 +16,7 @@ Actions and Services implicitly declare a new section. All commands 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 @@ locations on the system, described below. /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 @@ process all entries in the given fstab. 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 @@ Actions take the form of: +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 @@ Commands 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 ` > 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 = + for (file : files) + Import(file) Properties diff --git a/init/action.cpp b/init/action.cpp index 347edb054..c1289684d 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& args, - const std::string& filename, int line) - : func_(f), args_(args), filename_(filename), line_(line) { -} +Command::Command(BuiltinFunction f, const std::vector& args, int line) + : func_(f), args_(args), line_(line) {} int Command::InvokeFunc() const { std::vector expanded_args; @@ -49,21 +47,12 @@ std::string Command::BuildCommandString() const { 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* Action::function_map_ = nullptr; -bool Action::AddCommand(const std::vector& args, - const std::string& filename, int line, std::string* err) { +bool Action::AddCommand(const std::vector& args, int line, std::string* err) { if (!function_map_) { *err = "no function map available"; return false; @@ -79,20 +68,12 @@ bool Action::AddCommand(const std::vector& args, return false; } - AddCommand(function, args, filename, line); + AddCommand(function, args, line); return true; } -void Action::AddCommand(BuiltinFunction f, - const std::vector& 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& args, int line) { + commands_.emplace_back(f, args, line); } std::size_t Action::NumCommands() const { @@ -122,7 +103,7 @@ void Action::ExecuteCommand(const Command& command) const { 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 @@ bool Action::CheckPropertyTrigger(const std::string& name, 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 triggers; @@ -306,17 +282,7 @@ ActionManager& ActionManager::GetInstance() { } void ActionManager::AddAction(std::unique_ptr action) { - auto old_action_it = - std::find_if(actions_.begin(), actions_.end(), - [&action] (std::unique_ptr& 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 @@ void ActionManager::QueueAllPropertyTriggers() { QueuePropertyTrigger("", ""); } -void ActionManager::QueueBuiltinAction(BuiltinFunction func, - const std::string& name) { - auto action = std::make_unique(true); +void ActionManager::QueueBuiltinAction(BuiltinFunction func, const std::string& name) { + auto action = std::make_unique(true, "", 0); std::vector 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(action.get())); actions_.emplace_back(std::move(action)); @@ -366,7 +331,8 @@ void ActionManager::ExecuteOneCommand() { 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 @@ void ActionManager::DumpState() const { } } -bool ActionParser::ParseSection(const std::vector& args, - std::string* err) { +bool ActionParser::ParseSection(const std::vector& args, const std::string& filename, + int line, std::string* err) { std::vector triggers(args.begin() + 1, args.end()); if (triggers.size() < 1) { *err = "actions must have a trigger"; return false; } - auto action = std::make_unique(false); + auto action = std::make_unique(false, filename, line); if (!action->InitTriggers(triggers, err)) { return false; } @@ -414,10 +380,9 @@ bool ActionParser::ParseSection(const std::vector& args, return true; } -bool ActionParser::ParseLineSection(const std::vector& 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& 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 0bae9f0ef..25e5a3e22 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& args, - const std::string& filename, int line); + public: + Command(BuiltinFunction f, const std::vector& 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 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& args, - const std::string& filename, int line, std::string* err); - void AddCommand(BuiltinFunction f, - const std::vector& args, - const std::string& filename = "", int line = 0); - void CombineAction(const Action& action); + bool AddCommand(const std::vector& args, int line, std::string* err); + void AddCommand(BuiltinFunction f, const std::vector& args, int line); bool InitTriggers(const std::vector& args, std::string* err); bool InitSingleTrigger(const std::string& trigger); std::size_t NumCommands() const; @@ -60,11 +55,12 @@ public: 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* function_map) { function_map_ = function_map; } @@ -80,6 +76,8 @@ private: std::string event_trigger_; std::vector commands_; bool oneshot_; + std::string filename_; + int line_; static const KeywordMap* function_map_; }; @@ -115,18 +113,17 @@ private: }; class ActionParser : public SectionParser { -public: + public: ActionParser() : action_(nullptr) { } - bool ParseSection(const std::vector& args, + bool ParseSection(const std::vector& args, const std::string& filename, int line, std::string* err) override; - bool ParseLineSection(const std::vector& args, - const std::string& filename, int line, - std::string* err) const override; + bool ParseLineSection(const std::vector& args, int line, std::string* err) override; void EndSection() override; void EndFile(const std::string&) override { } -private: + + private: std::unique_ptr action_; }; diff --git a/init/import_parser.cpp b/init/import_parser.cpp index 8a2bcc291..f66b2bae6 100644 --- a/init/import_parser.cpp +++ b/init/import_parser.cpp @@ -20,8 +20,8 @@ #include "util.h" -bool ImportParser::ParseSection(const std::vector& args, - std::string* err) { +bool ImportParser::ParseSection(const std::vector& 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 0e91025f4..e15d55515 100644 --- a/init/import_parser.h +++ b/init/import_parser.h @@ -23,20 +23,20 @@ #include class ImportParser : public SectionParser { -public: + public: ImportParser() { } - bool ParseSection(const std::vector& args, + bool ParseSection(const std::vector& args, const std::string& filename, int line, std::string* err) override; - bool ParseLineSection(const std::vector& args, - const std::string& filename, int line, - std::string* err) const override { + bool ParseLineSection(const std::vector& args, int line, + std::string* err) override { return true; } void EndSection() override { } void EndFile(const std::string& filename) override; -private: + + private: std::vector imports_; }; diff --git a/init/init_parser.cpp b/init/init_parser.cpp index 53e670bd7..b425497b1 100644 --- a/init/init_parser.cpp +++ b/init/init_parser.cpp @@ -71,14 +71,13 @@ void Parser::ParseData(const std::string& filename, const std::string& data) { } 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 6935fdf75..64f0cacbe 100644 --- a/init/init_parser.h +++ b/init/init_parser.h @@ -26,11 +26,10 @@ class SectionParser { public: virtual ~SectionParser() { } - virtual bool ParseSection(const std::vector& args, - std::string* err) = 0; - virtual bool ParseLineSection(const std::vector& args, - const std::string& filename, int line, - std::string* err) const = 0; + virtual bool ParseSection(const std::vector& args, const std::string& filename, + int line, std::string* err) = 0; + virtual bool ParseLineSection(const std::vector& 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 4adbbf0c0..caf5785e5 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -157,6 +157,7 @@ Service::Service(const std::string& name, const std::vector& args) gid_(0), namespace_flags_(0), seclabel_(""), + onrestart_(false, "", 0), ioprio_class_(IoSchedClass_NONE), ioprio_pri_(0), priority_(0), @@ -180,6 +181,7 @@ Service::Service(const std::string& name, unsigned flags, uid_t uid, gid_t gid, capabilities_(capabilities), namespace_flags_(namespace_flags), seclabel_(seclabel), + onrestart_(false, "", 0), ioprio_class_(IoSchedClass_NONE), ioprio_pri_(0), priority_(0), @@ -438,7 +440,8 @@ bool Service::ParseOneshot(const std::vector& args, std::string* er bool Service::ParseOnrestart(const std::vector& args, std::string* err) { std::vector 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 @@ void ServiceManager::ReapAnyOutstandingChildren() { } } -bool ServiceParser::ParseSection(const std::vector& args, - std::string* err) { +bool ServiceParser::ParseSection(const std::vector& 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 @@ bool ServiceParser::ParseSection(const std::vector& args, return true; } -bool ServiceParser::ParseLineSection(const std::vector& args, - const std::string& filename, int line, - std::string* err) const { +bool ServiceParser::ParseLineSection(const std::vector& args, int line, + std::string* err) { return service_ ? service_->ParseLine(args, err) : false; } diff --git a/init/service.h b/init/service.h index 5e89b9f15..6fe0258bc 100644 --- a/init/service.h +++ b/init/service.h @@ -212,18 +212,15 @@ private: }; class ServiceParser : public SectionParser { -public: - ServiceParser() : service_(nullptr) { - } - bool ParseSection(const std::vector& args, + public: + ServiceParser() : service_(nullptr) {} + bool ParseSection(const std::vector& args, const std::string& filename, int line, std::string* err) override; - bool ParseLineSection(const std::vector& args, - const std::string& filename, int line, - std::string* err) const override; + bool ParseLineSection(const std::vector& 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_;