From 30a6f276fd8850b0a78689d7bff3cb06a18cb286 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Wed, 19 Apr 2017 15:31:58 -0700 Subject: [PATCH] init: clean up the SectionParser interface and Parser class Remove the dependency on Action and Service from what should be a generic Parser class. Make ActionParser, ImportParser, and ServiceParser take a pointer to their associated classes instead of accessing them through a singleton. Misc fixes to SectionParser Interface: 1) Make SectionParser::ParseLineSection() non-const as it always should have been. 2) Use Rvalue references where appropriate 3) Remove extra std::string& filename in SectionParser::EndFile() 4) Only have SectionParser::ParseSection() as pure virtual Document SectionParser. Make ImportParser report the filename and line number of failed imports. Make ServiceParser report the filename and line number of duplicated services. Test: Boot bullhead Change-Id: I86568a5b375fb4f27f4cb235ed1e37635f01d630 --- init/action.cpp | 9 ++++----- init/action.h | 11 +++++------ init/builtins.cpp | 2 +- init/import_parser.cpp | 14 ++++++++------ init/import_parser.h | 19 ++++++++----------- init/init.cpp | 22 ++++++++++++++-------- init/init.h | 2 ++ init/init_parser.cpp | 18 ++++++------------ init/init_parser.h | 38 +++++++++++++++++++++++++++++--------- init/service.cpp | 20 ++++++++++---------- init/service.h | 9 +++++---- 11 files changed, 92 insertions(+), 72 deletions(-) diff --git a/init/action.cpp b/init/action.cpp index c1289684d..8d49e2ad4 100644 --- a/init/action.cpp +++ b/init/action.cpp @@ -363,7 +363,7 @@ void ActionManager::DumpState() const { } } -bool ActionParser::ParseSection(const std::vector& args, const std::string& filename, +bool ActionParser::ParseSection(std::vector&& args, const std::string& filename, int line, std::string* err) { std::vector triggers(args.begin() + 1, args.end()); if (triggers.size() < 1) { @@ -380,13 +380,12 @@ bool ActionParser::ParseSection(const std::vector& args, const std: return true; } -bool ActionParser::ParseLineSection(const std::vector& args, int line, - std::string* err) { - return action_ ? action_->AddCommand(args, line, err) : false; +bool ActionParser::ParseLineSection(std::vector&& args, int line, std::string* err) { + return action_ ? action_->AddCommand(std::move(args), line, err) : false; } void ActionParser::EndSection() { if (action_ && action_->NumCommands() > 0) { - ActionManager::GetInstance().AddAction(std::move(action_)); + action_manager_->AddAction(std::move(action_)); } } diff --git a/init/action.h b/init/action.h index 25e5a3e22..e099b58d3 100644 --- a/init/action.h +++ b/init/action.h @@ -114,16 +114,15 @@ private: class ActionParser : public SectionParser { public: - ActionParser() : action_(nullptr) { - } - bool ParseSection(const std::vector& args, const std::string& filename, int line, + ActionParser(ActionManager* action_manager) + : action_manager_(action_manager), action_(nullptr) {} + bool ParseSection(std::vector&& args, const std::string& filename, int line, std::string* err) override; - bool ParseLineSection(const std::vector& args, int line, std::string* err) override; + bool ParseLineSection(std::vector&& args, int line, std::string* err) override; void EndSection() override; - void EndFile(const std::string&) override { - } private: + ActionManager* action_manager_; std::unique_ptr action_; }; diff --git a/init/builtins.cpp b/init/builtins.cpp index 43eb42075..87d4e81af 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -390,7 +390,7 @@ static void import_late(const std::vector& args, size_t start_index // Turning this on and letting the INFO logging be discarded adds 0.2s to // Nexus 9 boot time, so it's disabled by default. - if (false) parser.DumpState(); + if (false) DumpState(); } /* mount_fstab diff --git a/init/import_parser.cpp b/init/import_parser.cpp index f66b2bae6..99275e5e7 100644 --- a/init/import_parser.cpp +++ b/init/import_parser.cpp @@ -20,7 +20,7 @@ #include "util.h" -bool ImportParser::ParseSection(const std::vector& args, const std::string& filename, +bool ImportParser::ParseSection(std::vector&& args, const std::string& filename, int line, std::string* err) { if (args.size() != 2) { *err = "single argument needed for import\n"; @@ -35,16 +35,18 @@ bool ImportParser::ParseSection(const std::vector& args, const std: } LOG(INFO) << "Added '" << conf_file << "' to import list"; - imports_.emplace_back(std::move(conf_file)); + if (filename_.empty()) filename_ = filename; + imports_.emplace_back(std::move(conf_file), line); return true; } -void ImportParser::EndFile(const std::string& filename) { +void ImportParser::EndFile() { auto current_imports = std::move(imports_); imports_.clear(); - for (const auto& s : current_imports) { - if (!Parser::GetInstance().ParseConfig(s)) { - PLOG(ERROR) << "could not import file '" << s << "' from '" << filename << "'"; + for (const auto& [import, line_num] : current_imports) { + if (!parser_->ParseConfig(import)) { + PLOG(ERROR) << filename_ << ": " << line_num << ": Could not import file '" << import + << "'"; } } } diff --git a/init/import_parser.h b/init/import_parser.h index e15d55515..45cbfadf0 100644 --- a/init/import_parser.h +++ b/init/import_parser.h @@ -24,20 +24,17 @@ class ImportParser : public SectionParser { public: - ImportParser() { - } - bool ParseSection(const std::vector& args, const std::string& filename, int line, + ImportParser(Parser* parser) : parser_(parser) {} + bool ParseSection(std::vector&& args, const std::string& filename, int line, std::string* err) 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; + void EndFile() override; private: - std::vector imports_; + Parser* parser_; + // Store filename for later error reporting. + std::string filename_; + // Vector of imports and their line numbers for later error reporting. + std::vector> imports_; }; #endif diff --git a/init/init.cpp b/init/init.cpp index 9a4aa246e..33330e90e 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -96,6 +96,11 @@ static std::unique_ptr waiting_for_prop(nullptr); static std::string wait_prop_name; static std::string wait_prop_value; +void DumpState() { + ServiceManager::GetInstance().DumpState(); + ActionManager::GetInstance().DumpState(); +} + void register_epoll_handler(int fd, void (*fn)()) { epoll_event ev; ev.events = EPOLLIN; @@ -1398,10 +1403,13 @@ int main(int argc, char** argv) { const BuiltinFunctionMap function_map; Action::set_function_map(&function_map); + ActionManager& am = ActionManager::GetInstance(); + ServiceManager& sm = ServiceManager::GetInstance(); Parser& parser = Parser::GetInstance(); - parser.AddSectionParser("service",std::make_unique()); - parser.AddSectionParser("on", std::make_unique()); - parser.AddSectionParser("import", std::make_unique()); + + parser.AddSectionParser("service", std::make_unique(&sm)); + parser.AddSectionParser("on", std::make_unique(&am)); + parser.AddSectionParser("import", std::make_unique(&parser)); std::string bootscript = GetProperty("ro.boot.init_rc", ""); if (bootscript.empty()) { parser.ParseConfig("/init.rc"); @@ -1419,9 +1427,7 @@ int main(int argc, char** argv) { // Turning this on and letting the INFO logging be discarded adds 0.2s to // Nexus 9 boot time, so it's disabled by default. - if (false) parser.DumpState(); - - ActionManager& am = ActionManager::GetInstance(); + if (false) DumpState(); am.QueueEventTrigger("early-init"); @@ -1456,10 +1462,10 @@ int main(int argc, char** argv) { // By default, sleep until something happens. int epoll_timeout_ms = -1; - if (!(waiting_for_prop || ServiceManager::GetInstance().IsWaitingForExec())) { + if (!(waiting_for_prop || sm.IsWaitingForExec())) { am.ExecuteOneCommand(); } - if (!(waiting_for_prop || ServiceManager::GetInstance().IsWaitingForExec())) { + if (!(waiting_for_prop || sm.IsWaitingForExec())) { restart_processes(); // If there's a process that needs restarting, wake up in time for that. diff --git a/init/init.h b/init/init.h index 1da335030..6add75fba 100644 --- a/init/init.h +++ b/init/init.h @@ -34,4 +34,6 @@ int add_environment(const char* key, const char* val); bool start_waiting_for_property(const char *name, const char *value); +void DumpState(); + #endif /* _INIT_INIT_H */ diff --git a/init/init_parser.cpp b/init/init_parser.cpp index b425497b1..5c7af79ee 100644 --- a/init/init_parser.cpp +++ b/init/init_parser.cpp @@ -17,14 +17,12 @@ #include "init_parser.h" #include -#include #include #include -#include "action.h" #include "parser.h" -#include "service.h" +#include "util.h" Parser::Parser() { } @@ -71,13 +69,14 @@ 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, state.filename, state.line, &ret_err)) { + if (!section_parser->ParseSection(std::move(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.line, &ret_err)) { + if (!section_parser->ParseLineSection(std::move(args), state.line, &ret_err)) { parse_error(&state, "%s\n", ret_err.c_str()); } } @@ -100,8 +99,8 @@ bool Parser::ParseConfigFile(const std::string& path) { data.push_back('\n'); // TODO: fix parse_config. ParseData(path, data); - for (const auto& sp : section_parsers_) { - sp.second->EndFile(path); + for (const auto& [section_name, section_parser] : section_parsers_) { + section_parser->EndFile(); } LOG(VERBOSE) << "(Parsing " << path << " took " << t << ".)"; @@ -141,8 +140,3 @@ bool Parser::ParseConfig(const std::string& path) { } return ParseConfigFile(path); } - -void Parser::DumpState() const { - ServiceManager::GetInstance().DumpState(); - ActionManager::GetInstance().DumpState(); -} diff --git a/init/init_parser.h b/init/init_parser.h index 64f0cacbe..acceed4f7 100644 --- a/init/init_parser.h +++ b/init/init_parser.h @@ -22,22 +22,42 @@ #include #include +// SectionParser is an interface that can parse a given 'section' in init. +// +// You can implement up to 4 functions below, with ParseSection() being mandatory. +// The first two function return bool with false indicating a failure and has a std::string* err +// parameter into which an error string can be written. It will be reported along with the +// filename and line number of where the error occurred. +// +// 1) bool ParseSection(std::vector&& args, const std::string& filename, +// int line, std::string* err) +// This function is called when a section is first encountered. +// +// 2) bool ParseLineSection(std::vector&& args, int line, std::string* err) +// This function is called on each subsequent line until the next section is encountered. +// +// 3) bool EndSection() +// This function is called either when a new section is found or at the end of the file. +// It indicates that parsing of the current section is complete and any relevant objects should +// be committed. +// +// 4) bool EndFile() +// This function is called at the end of the file. +// It indicates that the parsing has completed and any relevant objects should be committed. + class SectionParser { -public: - virtual ~SectionParser() { - } - virtual bool ParseSection(const std::vector& args, const std::string& filename, + public: + virtual ~SectionParser() {} + virtual bool ParseSection(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; + virtual bool ParseLineSection(std::vector&&, int, std::string*) { return true; }; + virtual void EndSection(){}; + virtual void EndFile(){}; }; class Parser { public: static Parser& GetInstance(); - void DumpState() const; bool ParseConfig(const std::string& path); void AddSectionParser(const std::string& name, std::unique_ptr parser); diff --git a/init/service.cpp b/init/service.cpp index c0745e340..08d0a1d6c 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -877,11 +877,6 @@ ServiceManager& ServiceManager::GetInstance() { } void ServiceManager::AddService(std::unique_ptr service) { - Service* old_service = FindServiceByName(service->name()); - if (old_service) { - LOG(ERROR) << "ignored duplicate definition of service '" << service->name() << "'"; - return; - } services_.emplace_back(std::move(service)); } @@ -1095,7 +1090,7 @@ void ServiceManager::ReapAnyOutstandingChildren() { } } -bool ServiceParser::ParseSection(const std::vector& args, const std::string& filename, +bool ServiceParser::ParseSection(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"; @@ -1108,19 +1103,24 @@ bool ServiceParser::ParseSection(const std::vector& args, const std return false; } + Service* old_service = service_manager_->FindServiceByName(name); + if (old_service) { + *err = "ignored duplicate definition of service '" + name + "'"; + return false; + } + std::vector str_args(args.begin() + 2, args.end()); service_ = std::make_unique(name, str_args); return true; } -bool ServiceParser::ParseLineSection(const std::vector& args, int line, - std::string* err) { - return service_ ? service_->ParseLine(args, err) : false; +bool ServiceParser::ParseLineSection(std::vector&& args, int line, std::string* err) { + return service_ ? service_->ParseLine(std::move(args), err) : false; } void ServiceParser::EndSection() { if (service_) { - ServiceManager::GetInstance().AddService(std::move(service_)); + service_manager_->AddService(std::move(service_)); } } diff --git a/init/service.h b/init/service.h index 6fe0258bc..634fe4e63 100644 --- a/init/service.h +++ b/init/service.h @@ -213,16 +213,17 @@ private: class ServiceParser : public SectionParser { public: - ServiceParser() : service_(nullptr) {} - bool ParseSection(const std::vector& args, const std::string& filename, int line, + ServiceParser(ServiceManager* service_manager) + : service_manager_(service_manager), service_(nullptr) {} + bool ParseSection(std::vector&& args, const std::string& filename, int line, std::string* err) override; - bool ParseLineSection(const std::vector& args, int line, std::string* err) override; + bool ParseLineSection(std::vector&& args, int line, std::string* err) override; void EndSection() override; - void EndFile(const std::string&) override {} private: bool IsValidName(const std::string& name) const; + ServiceManager* service_manager_; std::unique_ptr service_; };