From 1eb97d9c3d3a35c6421d7e9831db14ba8e038d89 Mon Sep 17 00:00:00 2001 From: "William A. Kennington III" Date: Thu, 13 Sep 2018 00:36:12 -0700 Subject: Convert to standard CLI11 argument parser This simplifies the argument parsing logic drastically and still provides the same error handling as before. Tested: Ran through unit test suite and manually verified that the command line functions as expected on a real BMC. Change-Id: Ic5d69adf5359f9f64f2ada17e6a8f3242ca03e25 Signed-off-by: William A. Kennington III --- Makefile.am | 1 - argument.cpp | 127 ---------------------------- argument.hpp | 63 -------------- configure.ac | 5 ++ mainapp.cpp | 219 ++++++++++++++++++++++++------------------------- test/Makefile.am | 8 +- test/argument_test.cpp | 158 ----------------------------------- test/argument_test.hpp | 21 ----- 8 files changed, 113 insertions(+), 489 deletions(-) delete mode 100644 argument.cpp delete mode 100644 argument.hpp delete mode 100644 test/argument_test.cpp delete mode 100644 test/argument_test.hpp diff --git a/Makefile.am b/Makefile.am index 2de74ce..26c1980 100644 --- a/Makefile.am +++ b/Makefile.am @@ -3,7 +3,6 @@ sbin_PROGRAMS = phosphor-watchdog noinst_HEADERS = watchdog.hpp phosphor_watchdog_SOURCES = \ - argument.cpp \ watchdog.cpp \ mainapp.cpp diff --git a/argument.cpp b/argument.cpp deleted file mode 100644 index fa0d4bc..0000000 --- a/argument.cpp +++ /dev/null @@ -1,127 +0,0 @@ -/** - * Copyright © 2016 IBM Corporation - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include "argument.hpp" - -#include -#include - -namespace phosphor -{ -namespace watchdog -{ - -using namespace std::string_literals; - -const std::vector emptyArg; -const std::string ArgumentParser::trueString = "true"s; - -const char* ArgumentParser::optionStr = "p:s:t:a:f:i:ech"; -// clang-format off -const option ArgumentParser::options[] = -{ - { "path", required_argument, nullptr, 'p' }, - { "service", required_argument, nullptr, 's' }, - { "target", required_argument, nullptr, 't' }, - { "action_target", required_argument, nullptr, 'a' }, - { "fallback_action", required_argument, nullptr, 'f' }, - { "fallback_interval", required_argument, nullptr, 'i' }, - { "fallback_always", no_argument, nullptr, 'e' }, - { "continue", no_argument, nullptr, 'c' }, - { "help", no_argument, nullptr, 'h' }, - { 0, 0, 0, 0}, -}; -// clang-format on - -ArgumentParser::ArgumentParser(int argc, char* const argv[]) -{ - int option; - int opt_idx = 0; - - // We have to reset optind because getopt_long keeps global state - // and uses optind to track what argv index it needs to process next. - // Since this object may be instantiated more than once or test suites may - // already process instructions, optind may not be initialized to point to - // the beginning of our argv. - optind = 0; - while (-1 != - (option = getopt_long(argc, argv, optionStr, options, &opt_idx))) - { - if (option == '?' || option == 'h') - { - usage(argv); - exit(-1); - } - - auto i = &options[opt_idx]; - while (i->val && i->val != option) - { - ++i; - } - - if (i->val) - { - arguments[i->name].push_back(optarg ? optarg : trueString); - } - - opt_idx = 0; - } -} - -const std::vector& ArgumentParser:: - operator[](const std::string& opt) -{ - auto i = arguments.find(opt); - if (i == arguments.end()) - { - return emptyArg; - } - else - { - return i->second; - } -} - -void ArgumentParser::usage(char* const argv[]) -{ - std::cerr << "Usage: " << argv[0] << " options\n"; - std::cerr << "Options:\n"; - std::cerr << " --help Print this menu\n"; - std::cerr << " --path= Dbus Object path. " - "Ex: /xyz/openbmc_project/state/watchdog/host0\n"; - std::cerr << " --service= Dbus Service " - "name. Ex: xyz.openbmc_project.State.Watchdog.Host\n"; - std::cerr << " [--target=] Systemd unit to " - "be called on timeout for all actions but NONE. " - "Deprecated, use --action_target instead.\n"; - std::cerr << " [--action_target==] Map of action to " - "systemd unit to be called on timeout if that action is " - "set for ExpireAction when the timer expires.\n"; - std::cerr << " [--fallback_action=] Enables the " - "watchdog even when disabled via the dbus interface. " - "Perform this action when the fallback expires.\n"; - std::cerr << " [--fallback_interval=] Enables the " - "watchdog even when disabled via the dbus interface. " - "Waits for this interval before performing the fallback " - "action.\n"; - std::cerr << " [--fallback_always] Enables the " - "watchdog even when disabled by the dbus interface. " - "This option is only valid with a fallback specified.\n"; - std::cerr << " [--continue] Continue daemon " - "after watchdog timeout.\n"; -} -} // namespace watchdog -} // namespace phosphor diff --git a/argument.hpp b/argument.hpp deleted file mode 100644 index df61de1..0000000 --- a/argument.hpp +++ /dev/null @@ -1,63 +0,0 @@ -#pragma once - -#include - -#include -#include -#include - -namespace phosphor -{ -namespace watchdog -{ -/** @brief Class ArgumentParser - Encapsulates parsing command line options - * and populating arguments - */ -class ArgumentParser -{ - public: - ArgumentParser() = delete; - ~ArgumentParser() = default; - ArgumentParser(const ArgumentParser&) = delete; - ArgumentParser& operator=(const ArgumentParser&) = delete; - ArgumentParser(ArgumentParser&&) = default; - ArgumentParser& operator=(ArgumentParser&&) = default; - - /** @brief Constructs ArgumentParser object - * - * @param argc - the main function's argc passed as is - * @param argv - the main function's argv passed as is - * @return Object constructed - */ - ArgumentParser(int argc, char* const argv[]); - - /** @brief Given an option, returns its argument(optarg) - * - * @param opt - command line option string - * - * @return argument which is a standard optarg - */ - const std::vector& operator[](const std::string& opt); - - /** @brief Displays usage - * - * @param argv - the main function's argv passed as is - */ - static void usage(char* const argv[]); - - /** @brief Set to 'true' when an option is passed */ - static const std::string trueString; - - private: - /** @brief Option to argument mapping */ - std::map> arguments; - - /** @brief Array of struct options as needed by getopt_long */ - static const option options[]; - - /** @brief optstring as needed by getopt_long */ - static const char* optionStr; -}; - -} // namespace watchdog -} // namespace phosphor diff --git a/configure.ac b/configure.ac index cd4ae00..b1e9527 100644 --- a/configure.ac +++ b/configure.ac @@ -31,6 +31,11 @@ PKG_CHECK_MODULES([GTEST_MAIN], [gtest_main], [], AC_SUBST(GTEST_MAIN_LIBS) ]) +# We need the header only CLI library +AC_CHECK_HEADERS([CLI/CLI.hpp], [], [ + AC_MSG_ERROR([Could not find CLI11 CLI/CLI.hpp]) +]) + AC_ARG_ENABLE([oe-sdk], AS_HELP_STRING([--enable-oe-sdk], [Link testcases absolutely against OE SDK so they can be ran within it.]) ) diff --git a/mainapp.cpp b/mainapp.cpp index b98b108..1aa69e5 100644 --- a/mainapp.cpp +++ b/mainapp.cpp @@ -14,9 +14,9 @@ * limitations under the License. */ -#include "argument.hpp" #include "watchdog.hpp" +#include #include #include #include @@ -25,17 +25,9 @@ #include #include -using phosphor::watchdog::ArgumentParser; using phosphor::watchdog::Watchdog; using sdbusplus::xyz::openbmc_project::State::server::convertForMessage; -static void exitWithError(const char* err, char** argv) -{ - ArgumentParser::usage(argv); - std::cerr << "ERROR: " << err << "\n"; - exit(EXIT_FAILURE); -} - void printActionTargetMap(const Watchdog::ActionTargetMap& actionTargetMap) { std::cerr << "Action Targets:\n"; @@ -47,74 +39,107 @@ void printActionTargetMap(const Watchdog::ActionTargetMap& actionTargetMap) std::cerr << std::flush; } +void printFallback(const Watchdog::Fallback& fallback) +{ + std::cerr << "Fallback Options:\n"; + std::cerr << " Action: " << convertForMessage(fallback.action) << "\n"; + std::cerr << " Interval(ms): " << std::dec << fallback.interval << "\n"; + std::cerr << " Always re-execute: " << std::boolalpha << fallback.always + << "\n"; + std::cerr << std::flush; +} + int main(int argc, char* argv[]) { using namespace phosphor::logging; using InternalFailure = sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure; - // Read arguments. - auto options = ArgumentParser(argc, argv); - - // Parse out continue argument. - auto continueParam = (options)["continue"]; - // Default it to exit on watchdog timeout - auto continueAfterTimeout = false; - if (!continueParam.empty()) - { - continueAfterTimeout = true; - } - - // Parse out path argument. - auto pathParam = (options)["path"]; - if (pathParam.empty()) - { - exitWithError("Path not specified.", argv); - } - if (pathParam.size() > 1) - { - exitWithError("Multiple paths specified.", argv); - } - auto path = pathParam.back(); - - // Parse out service name argument - auto serviceParam = (options)["service"]; - if (serviceParam.empty()) - { - exitWithError("Service not specified.", argv); - } - if (serviceParam.size() > 1) - { - exitWithError("Multiple services specified.", argv); - } - auto service = serviceParam.back(); - // Parse out target argument. It is fine if the caller does not - // pass this if they are not interested in calling into any target - // on meeting a condition. - auto targetParam = (options)["target"]; - if (targetParam.size() > 1) - { - exitWithError("Multiple targets specified.", argv); - } + CLI::App app{"Canonical openbmc host watchdog daemon"}; + + // Service related options + const std::string serviceGroup = "Service Options"; + std::string path; + app.add_option("-p,--path", path, + "DBus Object Path. " + "Ex: /xyz/openbmc_project/state/watchdog/host0") + ->required() + ->group(serviceGroup); + std::string service; + app.add_option("-s,--service", service, + "DBus Service Name. " + "Ex: xyz.openbmc_project.State.Watchdog.Host") + ->required() + ->group(serviceGroup); + bool continueAfterTimeout; + app.add_flag("-c,--continue", continueAfterTimeout, + "Continue daemon after watchdog timeout") + ->group(serviceGroup); + + // Target related options + const std::string targetGroup = "Target Options"; + std::optional target; + app.add_option("-t,--target", target, + "Systemd unit to be called on " + "timeout for all actions but NONE. " + "Deprecated, use --action_target instead.") + ->group(targetGroup); + std::vector actionTargets; + app.add_option("-a,--action_target", actionTargets, + "Map of action to " + "systemd unit to be called on timeout if that action is " + "set for ExpireAction when the timer expires.") + ->group(targetGroup); + + // Fallback related options + const std::string fallbackGroup = "Fallback Options"; + std::optional fallbackAction; + auto fallbackActionOpt = + app.add_option("-f,--fallback_action", fallbackAction, + "Enables the " + "watchdog even when disabled via the dbus interface. " + "Perform this action when the fallback expires.") + ->group(fallbackGroup); + std::optional fallbackIntervalMs; + auto fallbackIntervalOpt = + app.add_option("-i,--fallback_interval", fallbackIntervalMs, + "Enables the " + "watchdog even when disabled via the dbus interface. " + "Waits for this interval before performing the fallback " + "action.") + ->group(fallbackGroup); + fallbackIntervalOpt->needs(fallbackActionOpt); + fallbackActionOpt->needs(fallbackIntervalOpt); + bool fallbackAlways; + app.add_flag("-e,--fallback_always", fallbackAlways, + "Enables the " + "watchdog even when disabled by the dbus interface. " + "This option is only valid with a fallback specified") + ->group(fallbackGroup) + ->needs(fallbackActionOpt) + ->needs(fallbackIntervalOpt); + + CLI11_PARSE(app, argc, argv); + + // Put together a list of actions and associated systemd targets + // The new --action_target options take precedence over the legacy + // --target Watchdog::ActionTargetMap actionTargetMap; - if (!targetParam.empty()) + if (target) { - auto target = targetParam.back(); - actionTargetMap[Watchdog::Action::HardReset] = target; - actionTargetMap[Watchdog::Action::PowerOff] = target; - actionTargetMap[Watchdog::Action::PowerCycle] = target; + actionTargetMap[Watchdog::Action::HardReset] = *target; + actionTargetMap[Watchdog::Action::PowerOff] = *target; + actionTargetMap[Watchdog::Action::PowerCycle] = *target; } - - // Parse out the action_target arguments. We allow one target to map - // to an action. These targets can replace the target specified above. - for (const auto& actionTarget : (options)["action_target"]) + for (const auto& actionTarget : actionTargets) { size_t keyValueSplit = actionTarget.find("="); if (keyValueSplit == std::string::npos) { - exitWithError( - "Invalid action_target format, expect =.", - argv); + std::cerr << "Invalid action_target format, " + "expect =." + << std::endl; + return 1; } std::string key = actionTarget.substr(0, keyValueSplit); @@ -128,72 +153,42 @@ int main(int argc, char* argv[]) } catch (const sdbusplus::exception::InvalidEnumString&) { - exitWithError("Bad action specified.", argv); + std::cerr << "Bad action specified: " << key << std::endl; + return 1; } // Detect duplicate action target arguments if (actionTargetMap.find(action) != actionTargetMap.end()) { - exitWithError("Duplicate action specified", argv); + std::cerr << "Got duplicate action: " << key << std::endl; + return 1; } actionTargetMap[action] = std::move(value); } printActionTargetMap(actionTargetMap); - // Parse out the fallback settings for the watchdog. Note that we require - // both of the fallback arguments to do anything here, but having a fallback - // is entirely optional. - auto fallbackActionParam = (options)["fallback_action"]; - auto fallbackIntervalParam = (options)["fallback_interval"]; - if (fallbackActionParam.empty() ^ fallbackIntervalParam.empty()) + // Build the fallback option used for the Watchdog + std::optional maybeFallback; + if (fallbackAction) { - exitWithError("Only one of the fallback options was specified.", argv); - } - if (fallbackActionParam.size() > 1 || fallbackIntervalParam.size() > 1) - { - exitWithError("Multiple fallbacks specified.", argv); - } - std::optional fallback; - if (!fallbackActionParam.empty()) - { - Watchdog::Action action; + Watchdog::Fallback fallback; try { - action = - Watchdog::convertActionFromString(fallbackActionParam.back()); + fallback.action = + Watchdog::convertActionFromString(*fallbackAction); } catch (const sdbusplus::exception::InvalidEnumString&) { - exitWithError("Bad action specified.", argv); + std::cerr << "Bad fallback action specified: " << *fallbackAction + << std::endl; + return 1; } - uint64_t interval; - try - { - interval = std::stoull(fallbackIntervalParam.back()); - } - catch (const std::logic_error&) - { - exitWithError("Failed to convert fallback interval to integer.", - argv); - } - fallback = Watchdog::Fallback{ - .action = action, - .interval = interval, - .always = false, - }; - } + fallback.interval = *fallbackIntervalMs; + fallback.always = fallbackAlways; - auto fallbackAlwaysParam = (options)["fallback_always"]; - if (!fallbackAlwaysParam.empty()) - { - if (!fallback) - { - exitWithError("Specified the fallback should always be enabled but " - "no fallback provided.", - argv); - } - fallback->always = true; + printFallback(fallback); + maybeFallback = std::move(fallback); } try @@ -212,7 +207,7 @@ int main(int argc, char* argv[]) // Create a watchdog object Watchdog watchdog(bus, path.c_str(), event, std::move(actionTargetMap), - std::move(fallback)); + std::move(maybeFallback)); // Claim the bus bus.request_name(service.c_str()); diff --git a/test/Makefile.am b/test/Makefile.am index 1c44162..bb1d048 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -4,8 +4,7 @@ AM_CPPFLAGS = -I$(top_srcdir) TESTS = $(check_PROGRAMS) # Build/add utest to test suite -check_PROGRAMS = argument_test \ - watchdog_test +check_PROGRAMS = watchdog_test utestCPPFLAGS = $(GTEST_MAIN_CFLAGS) \ $(AM_CPPFLAGS) \ @@ -24,13 +23,8 @@ utestLDFLAGS = $(GTEST_MAIN_LIBS) \ $(PHOSPHOR_LOGGING_LIBS) \ $(PHOSPHOR_DBUS_INTERFACES_LIBS) -argument_test_CPPFLAGS = ${utestCPPFLAGS} -argument_test_CXXFLAGS = ${utestCXXFLAGS} -argument_test_LDFLAGS = ${utestLDFLAGS} - watchdog_test_CPPFLAGS = ${utestCPPFLAGS} watchdog_test_CXXFLAGS = ${utestCXXFLAGS} watchdog_test_LDFLAGS = ${utestLDFLAGS} -argument_test_SOURCES = ../argument.cpp argument_test.cpp watchdog_test_SOURCES = ../watchdog.cpp watchdog_test.cpp diff --git a/test/argument_test.cpp b/test/argument_test.cpp deleted file mode 100644 index 3cb5dc9..0000000 --- a/test/argument_test.cpp +++ /dev/null @@ -1,158 +0,0 @@ -#include "argument_test.hpp" - -#include "argument.hpp" - -#include -#include - -static const std::string expected_path1 = "/arg1-test-path"; -static const std::string expected_target1 = "t1.target"; -static const std::string expected_target2 = "t2.target"; - -// Allow for a single unrecognized option then the Usage printout -static const std::string invalid_arg_regex = - "^[^\n]*unrecognized option[^\n]*\nUsage: "; - -static const std::string clean_usage_regex = "^Usage: "; - -namespace phosphor -{ -namespace watchdog -{ - -void ArgumentTest::SetUp() -{ - arg0 = "argument_test"; -} - -/** @brief ArgumentParser should return no values if given no options */ -TEST_F(ArgumentTest, NoOptions) -{ - char* const args[] = {&arg0[0], nullptr}; - ArgumentParser ap(sizeof(args) / sizeof(char*) - 1, args); - EXPECT_EQ(std::vector({}), ap["path"]); - EXPECT_EQ(std::vector({}), ap["continue"]); - EXPECT_EQ(std::vector({}), ap["arbitrary_unknown"]); -} - -/** @brief ArgumentParser should return true for an existing no-arg option - * Make sure we don't parse arguments if an option takes none - * Also make sure we don't populate options not used. - */ -TEST_F(ArgumentTest, LongOptionNoArg) -{ - std::string arg_continue = "--continue"; - std::string arg_extra = "not-a-bool"; - char* const args[] = {&arg0[0], &arg_continue[0], &arg_extra[0], nullptr}; - ArgumentParser ap(sizeof(args) / sizeof(char*) - 1, args); - EXPECT_EQ(std::vector({}), ap["path"]); - EXPECT_EQ(std::vector({ArgumentParser::trueString}), - ap["continue"]); -} - -/** @brief ArgumentParser should return a string for long options that - * take an arg - */ -TEST_F(ArgumentTest, LongOptionRequiredArg) -{ - std::string arg_path = "--path"; - std::string arg_path_val = expected_path1; - std::string arg_extra = "/unused-path"; - char* const args[] = {&arg0[0], &arg_path[0], &arg_path_val[0], - &arg_extra[0], nullptr}; - ArgumentParser ap(sizeof(args) / sizeof(char*) - 1, args); - EXPECT_EQ(std::vector({expected_path1}), ap["path"]); -} - -/** @brief ArgumentParser should return a string for long options that - * accept an argument when passed an argument inline - */ -TEST_F(ArgumentTest, LongOptionInlineArg) -{ - std::string arg_path = "--path=" + expected_path1; - std::string arg_extra = "/unused-path"; - char* const args[] = {&arg0[0], &arg_path[0], &arg_extra[0], nullptr}; - ArgumentParser ap(sizeof(args) / sizeof(char*) - 1, args); - EXPECT_EQ(std::vector({expected_path1}), ap["path"]); -} - -/** @brief ArgumentParser should return a string for short options that - * accept an argument. - */ -TEST_F(ArgumentTest, ShortOptionRequiredArg) -{ - std::string arg_path = "-p"; - std::string arg_path_val = expected_path1; - std::string arg_extra = "/unused-path"; - char* const args[] = {&arg0[0], &arg_path[0], &arg_path_val[0], - &arg_extra[0], nullptr}; - ArgumentParser ap(sizeof(args) / sizeof(char*) - 1, args); - EXPECT_EQ(std::vector({expected_path1}), ap["path"]); -} - -/** @brief ArgumentParser should be able to handle parsing multiple options - * Make sure this works for no-arg and required-arg type options - * Make sure this works between short and long options - */ -TEST_F(ArgumentTest, MultiOptionOverride) -{ - std::string arg_continue_short = "-c"; - std::string arg_path = "--path=" + expected_path1; - std::string arg_continue_long = "--continue"; - std::string arg_target = "--target=" + expected_target2; - std::string arg_target_short = "-t"; - std::string arg_target_val = expected_target1; - char* const args[] = {&arg0[0], &arg_continue_short[0], - &arg_path[0], &arg_continue_long[0], - &arg_target[0], &arg_target_short[0], - &arg_target_val[0], nullptr}; - ArgumentParser ap(sizeof(args) / sizeof(char*) - 1, args); - EXPECT_EQ(std::vector({expected_path1}), ap["path"]); - EXPECT_EQ(std::vector( - {ArgumentParser::trueString, ArgumentParser::trueString}), - ap["continue"]); - EXPECT_EQ(std::vector({expected_target2, expected_target1}), - ap["target"]); -} - -/** @brief ArgumentParser should print usage information when given a help - * argument anywhere in the arguments array - */ -TEST_F(ArgumentTest, ShortOptionHelp) -{ - std::string arg_extra = "extra"; - std::string arg_help = "-h"; - char* const args[] = {&arg0[0], &arg_extra[0], &arg_help[0], nullptr}; - EXPECT_EXIT(ArgumentParser(sizeof(args) / sizeof(char*) - 1, args), - ::testing::ExitedWithCode(255), clean_usage_regex); -} - -/** @brief ArgumentParser should print usage information when given a help - * argument anywhere in the arguments array - */ -TEST_F(ArgumentTest, LongOptionHelp) -{ - std::string arg_help = "--help"; - std::string arg_extra = "extra"; - char* const args[] = {&arg0[0], &arg_help[0], &arg_extra[0], nullptr}; - EXPECT_EXIT(ArgumentParser(sizeof(args) / sizeof(char*) - 1, args), - ::testing::ExitedWithCode(255), clean_usage_regex); -} - -/** @brief ArgumentParser should print an invalid argument error and - * usage information when given an invalid argument anywhere - * in the arguments array - */ -TEST_F(ArgumentTest, InvalidOptionHelp) -{ - std::string arg_continue = "--continue"; - std::string arg_bad = "--bad_arg"; - std::string arg_target = "--target=/unused-path"; - char* const args[] = {&arg0[0], &arg_continue[0], &arg_bad[0], - &arg_target[0], nullptr}; - EXPECT_EXIT(ArgumentParser(sizeof(args) / sizeof(char*) - 1, args), - ::testing::ExitedWithCode(255), invalid_arg_regex); -} - -} // namespace watchdog -} // namespace phosphor diff --git a/test/argument_test.hpp b/test/argument_test.hpp deleted file mode 100644 index 46e8762..0000000 --- a/test/argument_test.hpp +++ /dev/null @@ -1,21 +0,0 @@ -#pragma once - -#include - -#include - -namespace phosphor -{ -namespace watchdog -{ - -class ArgumentTest : public testing::Test -{ - protected: - void SetUp() override; - - std::string arg0; -}; - -} // namespace watchdog -} // namespace phosphor -- cgit v1.2.1