argument parser: use CLI11 and add unit tests
CLI11 is one of the most commonly use argument parser in OpenBMC. It can
save ~150 lines of codes in this project.
We are hitting argument related bugs that not covered in unit tests.
This test adds a test for argument parsing.
Tested: QEMU IPMI/Redfish worked.
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: Ib409c7e6a82ad31049f2da3e32727ebdf185f0fc
diff --git a/.gitignore b/.gitignore
index 4524188..db7a4a0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -4,3 +4,4 @@
!subprojects/phosphor-logging.wrap
!subprojects/sdbusplus.wrap
!subprojects/sdeventplus.wrap
+!subprojects/cli11.wrap
diff --git a/argument.cpp b/argument.cpp
index f0ee94d..ba4ebbc 100644
--- a/argument.cpp
+++ b/argument.cpp
@@ -1,90 +1,32 @@
-/**
- * Copyright © 2018 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 <cstdlib>
-#include <iostream>
-#include <utility>
+#include "certificate.hpp"
-namespace phosphor::certs::util
+#include <CLI/CLI.hpp>
+
+namespace phosphor::certs
{
-ArgumentParser::ArgumentParser(int argc, char** argv)
+int processArguments(int argc, const char* const* argv, Arguments& arguments)
{
- auto option = 0;
- while (-1 !=
- (option = getopt_long(argc, argv, optionstr, options, nullptr)))
+ CLI::App app{"OpenBMC Certificate Management Daemon"};
+ app.add_option("-t,--type", arguments.typeStr, "certificate type")
+ ->required();
+ app.add_option("-e,--endpoint", arguments.endpoint, "d-bus endpoint")
+ ->required();
+ app.add_option("-p,--path", arguments.path, "certificate file path")
+ ->required();
+ app.add_option("-u,--unit", arguments.unit,
+ "Optional systemd unit need to reload")
+ ->capture_default_str();
+ CLI11_PARSE(app, argc, argv);
+ phosphor::certs::CertificateType type =
+ phosphor::certs::stringToCertificateType(arguments.typeStr);
+ if (type == phosphor::certs::CertificateType::Unsupported)
{
- if ((option == '?') || (option == 'h'))
- {
- usage(argv);
- exit(-1);
- }
-
- auto i = &options[0];
- while ((i->val != option) && (i->val != 0))
- {
- ++i;
- }
-
- if (i->val)
- {
- arguments[i->name] = (i->has_arg ? optarg : true_string);
- }
+ std::cerr << "type not specified or invalid." << std::endl;
+ return 1;
}
+ return 0;
}
-
-const std::string& ArgumentParser::operator[](const std::string& opt)
-{
- auto i = arguments.find(opt);
- if (i == arguments.end())
- {
- return empty_string;
- }
- else
- {
- return i->second;
- }
-}
-
-void ArgumentParser::usage(char** argv)
-{
- std::cerr << "Usage: " << argv[0] << " [options]\n";
- std::cerr << "Options:\n";
- std::cerr << " --help Print this menu\n";
- std::cerr << " --type certificate type\n";
- std::cerr << " Valid types: client,server,authority\n";
- std::cerr << " --endpoint d-bus endpoint\n";
- std::cerr << " --path certificate file path\n";
- std::cerr << " --unit=<name> Optional systemd unit need to reload\n";
- std::cerr << std::flush;
-}
-
-const option ArgumentParser::options[] = {
- {"type", required_argument, nullptr, 't'},
- {"endpoint", required_argument, nullptr, 'e'},
- {"path", required_argument, nullptr, 'p'},
- {"unit", optional_argument, nullptr, 'u'},
- {"help", no_argument, nullptr, 'h'},
- {0, 0, 0, 0},
-};
-
-const char* ArgumentParser::optionstr = "tepuh?";
-
-const std::string ArgumentParser::true_string = "true";
-const std::string ArgumentParser::empty_string = "";
-
-} // namespace phosphor::certs::util
+} // namespace phosphor::certs
diff --git a/argument.hpp b/argument.hpp
index e67a877..5d0b558 100644
--- a/argument.hpp
+++ b/argument.hpp
@@ -1,39 +1,19 @@
#pragma once
-#include <getopt.h>
-
-#include <map>
#include <string>
-namespace phosphor::certs::util
+namespace phosphor::certs
{
-/**
- * @brief Class - Encapsulates parsing command line options and
- * populating arguments.
- */
-class ArgumentParser
+struct Arguments
{
- public:
- ArgumentParser(int argc, char** argv);
- ArgumentParser() = delete;
- ArgumentParser(const ArgumentParser&) = delete;
- ArgumentParser(ArgumentParser&&) = default;
- ArgumentParser& operator=(const ArgumentParser&) = delete;
- ArgumentParser& operator=(ArgumentParser&&) = default;
- ~ArgumentParser() = default;
- const std::string& operator[](const std::string& opt);
-
- static void usage(char** argv);
-
- static const std::string true_string;
- static const std::string empty_string;
-
- private:
- std::map<const std::string, std::string> arguments;
-
- static const option options[];
- static const char* optionstr;
+ std::string typeStr; // certificate type
+ std::string endpoint; // d-bus endpoint
+ std::string path; // certificate file path
+ std::string unit; // Optional systemd unit need to reload
};
-} // namespace phosphor::certs::util
+// Validates all |argv| is valid and set corresponding attributes in
+// |arguments|.
+int processArguments(int argc, const char* const* argv, Arguments& arguments);
+} // namespace phosphor::certs
diff --git a/dist/phosphor-certificate-manager@.service b/dist/phosphor-certificate-manager@.service
index 3b841fe..8edbb4e 100644
--- a/dist/phosphor-certificate-manager@.service
+++ b/dist/phosphor-certificate-manager@.service
@@ -2,8 +2,9 @@
Description=Phosphor certificate manager for %I
[Service]
+Environment=UNIT=""
EnvironmentFile=/usr/share/phosphor-certificate-manager/%I
-ExecStart=/usr/bin/env phosphor-certificate-manager --endpoint=${ENDPOINT} --path=${CERTPATH} --unit=${UNIT} --type=${TYPE}
+ExecStart=/usr/bin/env phosphor-certificate-manager --endpoint ${ENDPOINT} --path ${CERTPATH} --type ${TYPE} --unit ${UNIT}
SyslogIdentifier=phosphor-certificate-manager
Restart=always
UMask=0007
diff --git a/mainapp.cpp b/mainapp.cpp
index 971d327..c6b9a7d 100644
--- a/mainapp.cpp
+++ b/mainapp.cpp
@@ -20,25 +20,15 @@
#include "certificate.hpp"
#include "certs_manager.hpp"
-#include <stdlib.h>
#include <systemd/sd-event.h>
#include <cctype>
-#include <iostream>
#include <sdbusplus/bus.hpp>
#include <sdbusplus/server/manager.hpp>
#include <sdeventplus/event.hpp>
#include <string>
#include <utility>
-static void exitWithError(const char* err, char** argv)
-{
- phosphor::certs::util::ArgumentParser::usage(argv);
- std::cerr << std::endl;
- std::cerr << "ERROR: " << err << std::endl;
- exit(EXIT_FAILURE);
-}
-
inline std::string capitalize(const std::string& s)
{
std::string res = s;
@@ -51,36 +41,15 @@
int main(int argc, char** argv)
{
- // Read arguments.
- auto options = phosphor::certs::util::ArgumentParser(argc, argv);
-
- // Parse arguments
- const std::string& typeStr = (options)["type"];
- phosphor::certs::CertificateType type =
- phosphor::certs::stringToCertificateType(typeStr);
- if (type == phosphor::certs::CertificateType::Unsupported)
+ phosphor::certs::Arguments arguments;
+ if (phosphor::certs::processArguments(argc, argv, arguments) != 0)
{
- exitWithError("type not specified or invalid.", argv);
+ std::exit(EXIT_FAILURE);
}
- const std::string& endpoint = (options)["endpoint"];
- if (endpoint == phosphor::certs::util::ArgumentParser::empty_string)
- {
- exitWithError("endpoint not specified.", argv);
- }
-
- const std::string& path = (options)["path"];
- if (path == phosphor::certs::util::ArgumentParser::empty_string)
- {
- exitWithError("path not specified.", argv);
- }
-
- // unit is an optional parameter
- const std::string& unit = (options)["unit"];
auto bus = sdbusplus::bus::new_default();
- auto objPath =
- std::string(objectNamePrefix) + '/' + typeStr + '/' + endpoint;
-
+ auto objPath = std::string(objectNamePrefix) + '/' + arguments.typeStr +
+ '/' + arguments.endpoint;
// Add sdbusplus ObjectManager
sdbusplus::server::manager::manager objManager(bus, objPath.c_str());
@@ -89,13 +58,15 @@
// Attach the bus to sd_event to service user requests
bus.attach_event(event.get(), SD_EVENT_PRIORITY_NORMAL);
-
- phosphor::certs::Manager manager(bus, event, objPath.c_str(), type, unit,
- path);
+ phosphor::certs::Manager manager(
+ bus, event, objPath.c_str(),
+ phosphor::certs::stringToCertificateType(arguments.typeStr),
+ arguments.unit, arguments.path);
// Adjusting Interface name as per std convention
- auto busName = std::string(busNamePrefix) + '.' + capitalize(typeStr) +
- '.' + capitalize(endpoint);
+ auto busName = std::string(busNamePrefix) + '.' +
+ capitalize(arguments.typeStr) + '.' +
+ capitalize(arguments.endpoint);
bus.request_name(busName.c_str());
event.loop();
return 0;
diff --git a/meson.build b/meson.build
index 0720247..e2a337f 100644
--- a/meson.build
+++ b/meson.build
@@ -17,6 +17,18 @@
phosphor_dbus_interfaces_dep = dependency('phosphor-dbus-interfaces')
phosphor_logging_dep = dependency('phosphor-logging')
+cli11_dep = dependency('cli11', required: false)
+has_cli11 = meson.get_compiler('cpp').has_header_symbol(
+ 'CLI/CLI.hpp',
+ 'CLI::App',
+ dependencies: cli11_dep,
+ required: false)
+if not has_cli11
+ cli11_proj = subproject('cli11', required: false)
+ assert(cli11_proj.found(), 'CLI11 is required')
+ cli11_dep = cli11_proj.get_variable('CLI11_dep')
+endif
+
systemd_dep = dependency('systemd')
openssl_dep = dependency('openssl')
@@ -42,6 +54,7 @@
phosphor_logging_dep,
sdbusplus_dep,
sdeventplus_dep,
+ cli11_dep,
]
cert_manager_lib = static_library(
diff --git a/subprojects/cli11.wrap b/subprojects/cli11.wrap
new file mode 100644
index 0000000..407409c
--- /dev/null
+++ b/subprojects/cli11.wrap
@@ -0,0 +1,3 @@
+[wrap-git]
+url = https://github.com/CLIUtils/CLI11
+revision = HEAD
diff --git a/test/argument_test.cpp b/test/argument_test.cpp
new file mode 100644
index 0000000..d55abfa
--- /dev/null
+++ b/test/argument_test.cpp
@@ -0,0 +1,111 @@
+#include "argument.hpp"
+
+#include <string>
+#include <vector>
+
+#include <gtest/gtest.h>
+
+namespace phosphor::certs
+{
+namespace
+{
+
+TEST(ProcessArguments, OnSuccessClientType)
+{
+ Arguments arguments;
+ std::vector<const char*> argv = {"binary", "--type", "client",
+ "--endpoint", "abc", "--path",
+ "def", "--unit", "ghi"};
+ EXPECT_EQ(processArguments(argv.size(), argv.data(), arguments), 0);
+ EXPECT_EQ(arguments.typeStr, "client");
+ EXPECT_EQ(arguments.endpoint, "abc");
+ EXPECT_EQ(arguments.path, "def");
+ EXPECT_EQ(arguments.unit, "ghi");
+}
+
+TEST(ProcessArguments, OnSuccessServerType)
+{
+ Arguments arguments;
+ std::vector<const char*> argv = {"binary", "--type", "server",
+ "--endpoint", "abc", "--path",
+ "def", "--unit", "ghi"};
+ EXPECT_EQ(processArguments(argv.size(), argv.data(), arguments), 0);
+ EXPECT_EQ(arguments.typeStr, "server");
+ EXPECT_EQ(arguments.endpoint, "abc");
+ EXPECT_EQ(arguments.path, "def");
+ EXPECT_EQ(arguments.unit, "ghi");
+}
+
+TEST(ProcessArguments, OnSuccessAuthorityType)
+{
+ Arguments arguments;
+ std::vector<const char*> argv = {"binary", "--type", "authority",
+ "--endpoint", "abc", "--path",
+ "def", "--unit", "ghi"};
+ EXPECT_NO_THROW(processArguments(argv.size(), argv.data(), arguments));
+ EXPECT_EQ(arguments.typeStr, "authority");
+ EXPECT_EQ(arguments.endpoint, "abc");
+ EXPECT_EQ(arguments.path, "def");
+ EXPECT_EQ(arguments.unit, "ghi");
+}
+
+TEST(ProcessArguments, UnitIsOptional)
+{
+ Arguments arguments;
+ std::vector<const char*> argv = {"binary", "--type", "client", "--endpoint",
+ "abc", "--path", "def"};
+ EXPECT_EQ(processArguments(argv.size(), argv.data(), arguments), 0);
+ EXPECT_EQ(arguments.typeStr, "client");
+ EXPECT_EQ(arguments.endpoint, "abc");
+ EXPECT_EQ(arguments.path, "def");
+ EXPECT_TRUE(arguments.unit.empty());
+}
+
+TEST(ProcessArguments, EmptyUnit)
+{
+ Arguments arguments;
+ std::vector<const char*> argv = {"binary", "--type", "client",
+ "--endpoint", "abc", "--path",
+ "def", "--unit", ""};
+ EXPECT_EQ(processArguments(argv.size(), argv.data(), arguments), 0);
+ EXPECT_EQ(arguments.typeStr, "client");
+ EXPECT_EQ(arguments.endpoint, "abc");
+ EXPECT_EQ(arguments.path, "def");
+ EXPECT_TRUE(arguments.unit.empty());
+}
+
+TEST(Type, MissTypeThrows)
+{
+ Arguments arguments;
+ std::vector<const char*> argv = {"binary", "--endpoint", "abc", "--path",
+ "def", "--unit", "ghi"};
+ EXPECT_NE(processArguments(argv.size(), argv.data(), arguments), 0);
+}
+
+TEST(Type, WrongTypeThrows)
+{
+ Arguments arguments;
+ std::vector<const char*> argv = {"binary", "--type", "no-supported",
+ "--endpoint", "abc", "--path",
+ "def", "--unit", "ghi"};
+ EXPECT_NE(processArguments(argv.size(), argv.data(), arguments), 0);
+}
+
+TEST(Endpoint, MissEndpointThrows)
+{
+ Arguments arguments;
+ std::vector<const char*> argv = {"binary", "--type", "client", "--path",
+ "def", "--unit", "ghi"};
+ EXPECT_NE(processArguments(argv.size(), argv.data(), arguments), 0);
+}
+
+TEST(Path, MissPathThrows)
+{
+ Arguments arguments;
+ std::vector<const char*> argv = {"binary", "--type", "client", "--endpoint",
+ "abc", "--unit", "ghi"};
+ EXPECT_NE(processArguments(argv.size(), argv.data(), arguments), 0);
+}
+} // namespace
+
+} // namespace phosphor::certs
diff --git a/test/meson.build b/test/meson.build
index f3996fe..e2cc092 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -20,6 +20,20 @@
endif
test(
+ 'test_argument',
+ executable(
+ 'argument_test',
+ 'argument_test.cpp',
+ include_directories: '..',
+ dependencies: [
+ gtest_dep,
+ gmock_dep,
+ cert_manager_dep,
+ ],
+ ),
+)
+
+test(
'test_certs_manager',
executable(
'test-certs-manager',