From a4c92848be7f9ba0033e9759ed52c282d1fa1440 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Wed, 18 Nov 2015 11:01:48 +0100 Subject: [PATCH] Fix handling of relative paths and add test cases --- ChangeLog.txt | 1 + src/cli/Cli.cpp | 1 + src/cli/program_options/Parser.cpp | 20 ++++++++++++------ test/cli/CliTest_ShowingHelp.cpp | 4 ++-- test/cli/program_options/ParserTest.cpp | 21 +++++++++++++++++++ .../program_options/ProgramOptionsTest.cpp | 17 +++++++++++++-- test/cli/testutils/CliTest.h | 4 ++-- 7 files changed, 56 insertions(+), 12 deletions(-) diff --git a/ChangeLog.txt b/ChangeLog.txt index 40c31cfd..c26934b1 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -1,6 +1,7 @@ Version 0.8.2 --------------- * Mount directory and base directory can be specified as relative paths. +* Improved error messages. Version 0.8.1 --------------- diff --git a/src/cli/Cli.cpp b/src/cli/Cli.cpp index 6046550c..30dc9536 100644 --- a/src/cli/Cli.cpp +++ b/src/cli/Cli.cpp @@ -63,6 +63,7 @@ using boost::chrono::milliseconds; //TODO Improve parallelity. //TODO Did deadlock in bonnie++ second run (in the create files sequentially) - maybe also in a later run or different step? //TODO Replace ASSERTs with other error handling when it is not a programming error but an environment influence (e.g. a block is missing) +//TODO When calling cryfs without basedir parameter (i.e. just calling "cryfs"): the option '--base-dir' is required but missing. Should be a better error message, since --base-dir is not actually a command line option. namespace cryfs { diff --git a/src/cli/program_options/Parser.cpp b/src/cli/program_options/Parser.cpp index 7fe76d29..55b69e13 100644 --- a/src/cli/program_options/Parser.cpp +++ b/src/cli/program_options/Parser.cpp @@ -30,11 +30,19 @@ ProgramOptions Parser::parse(const vector &supportedCiphers) const { pair, vector> options = splitAtDoubleDash(_options); po::variables_map vm = _parseOptionsOrShowHelp(options.first, supportedCiphers); - bf::path baseDir = bf::canonical(vm["base-dir"].as()); - bf::path mountDir = bf::canonical(vm["mount-dir"].as()); + if (!vm.count("base-dir")) { + std::cerr << "Please specify a base directory.\n"; + _showHelpAndExit(); + } + if (!vm.count("mount-dir")) { + std::cerr << "Please specify a mount directory.\n"; + _showHelpAndExit(); + } + bf::path baseDir = bf::absolute(vm["base-dir"].as()); + bf::path mountDir = bf::absolute(vm["mount-dir"].as()); optional configfile = none; if (vm.count("config")) { - configfile = bf::canonical(vm["config"].as()); + configfile = bf::absolute(vm["config"].as()); } bool foreground = vm.count("foreground"); if (foreground) { @@ -46,7 +54,7 @@ ProgramOptions Parser::parse(const vector &supportedCiphers) const { } optional logfile = none; if (vm.count("logfile")) { - logfile = bf::canonical(vm["logfile"].as()); + logfile = bf::absolute(vm["logfile"].as()); } optional cipher = none; if (vm.count("cipher")) { @@ -117,8 +125,8 @@ void Parser::_addPositionalOptionForBaseDir(po::options_description *desc, po::p positional->add("mount-dir", 1); po::options_description hidden("Hidden options"); hidden.add_options() - ("base-dir", po::value()->required(), "Base directory") - ("mount-dir", po::value()->required(), "Mount directory") + ("base-dir", po::value(), "Base directory") + ("mount-dir", po::value(), "Mount directory") ; desc->add(hidden); } diff --git a/test/cli/CliTest_ShowingHelp.cpp b/test/cli/CliTest_ShowingHelp.cpp index 55c5a607..e701d278 100644 --- a/test/cli/CliTest_ShowingHelp.cpp +++ b/test/cli/CliTest_ShowingHelp.cpp @@ -19,9 +19,9 @@ TEST_F(CliTest_ShowingHelp, HelpShortOptionTogetherWithOtherOptions) { } TEST_F(CliTest_ShowingHelp, MissingAllOptions) { - EXPECT_EXIT_WITH_HELP_MESSAGE({}); + EXPECT_EXIT_WITH_HELP_MESSAGE({}, "Please specify a base directory"); } TEST_F(CliTest_ShowingHelp, MissingDir) { - EXPECT_EXIT_WITH_HELP_MESSAGE({basedir.c_str()}); + EXPECT_EXIT_WITH_HELP_MESSAGE({basedir.c_str()}, "Please specify a mount directory"); } diff --git a/test/cli/program_options/ParserTest.cpp b/test/cli/program_options/ParserTest.cpp index 83c84356..d3277271 100644 --- a/test/cli/program_options/ParserTest.cpp +++ b/test/cli/program_options/ParserTest.cpp @@ -6,6 +6,7 @@ using namespace cryfs; using namespace cryfs::program_options; using std::vector; using boost::none; +namespace bf = boost::filesystem; class ProgramOptionsParserTest: public ProgramOptionsTestBase { public: @@ -61,16 +62,36 @@ TEST_F(ProgramOptionsParserTest, NoSpecialOptions) { EXPECT_VECTOR_EQ({"./myExecutable", "/home/user/mountDir"}, options.fuseOptions()); } +TEST_F(ProgramOptionsParserTest, RelativeBaseDir) { + ProgramOptions options = parse({"./myExecutable", "baseDir", "/home/user/mountDir"}); + EXPECT_EQ(bf::current_path() / "baseDir", options.baseDir()); +} + +TEST_F(ProgramOptionsParserTest, RelativeMountDir) { + ProgramOptions options = parse({"./myExecutable", "/home/user/baseDir", "mountDir"}); + EXPECT_EQ(bf::current_path() / "mountDir", options.mountDir()); +} + TEST_F(ProgramOptionsParserTest, LogfileGiven) { ProgramOptions options = parse({"./myExecutable", "/home/user/baseDir", "--logfile", "/home/user/mylogfile", "/home/user/mountDir"}); EXPECT_EQ("/home/user/mylogfile", options.logFile().value()); } +TEST_F(ProgramOptionsParserTest, LogfileGiven_RelativePath) { + ProgramOptions options = parse({"./myExecutable", "/home/user/baseDir", "--logfile", "mylogfile", "/home/user/mountDir"}); + EXPECT_EQ(bf::current_path() / "mylogfile", options.logFile().value()); +} + TEST_F(ProgramOptionsParserTest, ConfigfileGiven) { ProgramOptions options = parse({"./myExecutable", "/home/user/baseDir", "--config", "/home/user/myconfigfile", "/home/user/mountDir"}); EXPECT_EQ("/home/user/myconfigfile", options.configFile().value()); } +TEST_F(ProgramOptionsParserTest, ConfigfileGiven_RelativePath) { + ProgramOptions options = parse({"./myExecutable", "/home/user/baseDir", "--config", "myconfigfile", "/home/user/mountDir"}); + EXPECT_EQ(bf::current_path() / "myconfigfile", options.configFile().value()); +} + TEST_F(ProgramOptionsParserTest, CipherGiven) { ProgramOptions options = parse({"./myExecutable", "/home/user/baseDir", "--cipher", "aes-256-gcm", "/home/user/mountDir"}); EXPECT_EQ("aes-256-gcm", options.cipher().value()); diff --git a/test/cli/program_options/ProgramOptionsTest.cpp b/test/cli/program_options/ProgramOptionsTest.cpp index 4bcbec9c..fa616f5a 100644 --- a/test/cli/program_options/ProgramOptionsTest.cpp +++ b/test/cli/program_options/ProgramOptionsTest.cpp @@ -5,7 +5,20 @@ using namespace cryfs::program_options; using std::vector; using boost::none; +using boost::optional; +using std::ostream; using std::string; +namespace bf = boost::filesystem; + +// This is needed for google test to work with boost::optional +namespace boost { + template<> ostream& operator<< , bf::path>(ostream &stream, const optional &path) { + if (path == none) { + return stream << "none"; + } + return stream << *path; + } +} class ProgramOptionsTest: public ProgramOptionsTestBase {}; @@ -25,7 +38,7 @@ TEST_F(ProgramOptionsTest, ConfigfileNone) { } TEST_F(ProgramOptionsTest, ConfigfileSome) { - ProgramOptions testobj("", "", string("/home/user/configfile"), true, none, none, none, none, options({"./myExecutable"})); + ProgramOptions testobj("", "", bf::path("/home/user/configfile"), true, none, none, none, none, options({"./myExecutable"})); EXPECT_EQ("/home/user/configfile", testobj.configFile().get()); } @@ -45,7 +58,7 @@ TEST_F(ProgramOptionsTest, LogfileNone) { } TEST_F(ProgramOptionsTest, LogfileSome) { - ProgramOptions testobj("", "", none, true, none, string("logfile"), none, none, options({"./myExecutable"})); + ProgramOptions testobj("", "", none, true, none, bf::path("logfile"), none, none, options({"./myExecutable"})); EXPECT_EQ("logfile", testobj.logFile().get()); } diff --git a/test/cli/testutils/CliTest.h b/test/cli/testutils/CliTest.h index e18bbf19..7826160c 100644 --- a/test/cli/testutils/CliTest.h +++ b/test/cli/testutils/CliTest.h @@ -32,8 +32,8 @@ public: cryfs::Cli(keyGenerator, cpputils::SCrypt::TestSettings).main(_args.size(), _args.data()); } - void EXPECT_EXIT_WITH_HELP_MESSAGE(std::vector args) { - EXPECT_RUN_ERROR(args, "Usage"); + void EXPECT_EXIT_WITH_HELP_MESSAGE(std::vector args, const std::string &message = "") { + EXPECT_RUN_ERROR(args, (message+".*Usage").c_str()); } void EXPECT_RUN_ERROR(std::vector args, const char *message) {