From: Francis Dupont Date: Fri, 16 May 2025 08:27:57 +0000 (+0200) Subject: [#3831] Checkpoint: fixes, still UTs to add X-Git-Tag: Kea-2.7.9~29 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=78c0cd0de373e2ef8518229aea558a7b78afa8d8;p=thirdparty%2Fkea.git [#3831] Checkpoint: fixes, still UTs to add --- diff --git a/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc b/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc index 64492a3c60..64814bf010 100644 --- a/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc +++ b/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc @@ -3490,13 +3490,13 @@ void Lease4CmdsTest::testLease4Write() { "{\n" " \"command\": \"lease4-write\",\n" " \"arguments\": {" - " \"filename\": \"/tmp/myleases.txt\"\n" + " \"filename\": \"/foo-bar/myleases.txt\"\n" " }\n" "}"; std::ostringstream os; os << "'filename' parameter is invalid: invalid path specified:" - << " '/tmp', supported path is '" << CfgMgr::instance().getDataDir() << "'"; + << " '/foo-bar', supported path is '" << CfgMgr::instance().getDataDir() << "'"; testCommand(txt, CONTROL_RESULT_ERROR, os.str()); } diff --git a/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds6_unittest.cc b/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds6_unittest.cc index 0260d26886..cae59227e6 100644 --- a/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds6_unittest.cc +++ b/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds6_unittest.cc @@ -4452,13 +4452,13 @@ void Lease6CmdsTest::testLease6Write() { "{\n" " \"command\": \"lease6-write\",\n" " \"arguments\": {" - " \"filename\": \"/tmp/myleases.txt\"\n" + " \"filename\": \"/foo-bar/myleases.txt\"\n" " }\n" "}"; std::ostringstream os; os << "'filename' parameter is invalid: invalid path specified:" - << " '/tmp', supported path is '" << CfgMgr::instance().getDataDir() << "'"; + << " '/foo-bar', supported path is '" << CfgMgr::instance().getDataDir() << "'"; } TEST_F(Lease6CmdsTest, lease6AddMissingParams) { diff --git a/src/lib/process/daemon.cc b/src/lib/process/daemon.cc index e5d517979f..cba9ded080 100644 --- a/src/lib/process/daemon.cc +++ b/src/lib/process/daemon.cc @@ -134,13 +134,13 @@ Daemon::checkWriteConfigFile(std::string& file) { << " is missing file name"); } Path current(config_file_); - if (current.parentPath() == path.parentPath()) { - // Same paths! + if (current.parentDirectory() == path.parentDirectory()) { + // Same parent directories! return; } - if (path.parentPath().empty()) { - // Note the current path can't be empty here. - file = current.parentPath() + "/" + file; + if (path.parentDirectory().empty()) { + // Note the current parent directory can't be empty here. + file = current.parentDirectory() + file; return; } isc_throw(isc::BadValue, "file " << file << " must be in the same " diff --git a/src/lib/process/tests/daemon_unittest.cc b/src/lib/process/tests/daemon_unittest.cc index 3430e21b4a..0cfbf4c776 100644 --- a/src/lib/process/tests/daemon_unittest.cc +++ b/src/lib/process/tests/daemon_unittest.cc @@ -40,8 +40,8 @@ std::string DaemonImpl::getVersion(bool extended) { } } -}; -}; +} +} namespace { @@ -77,7 +77,6 @@ private: std::string env_copy_; }; - // Very simple test. Checks whether Daemon can be instantiated and its // default parameters are sane TEST_F(DaemonTest, constructor) { @@ -113,6 +112,47 @@ TEST_F(DaemonTest, checkConfigFile) { EXPECT_NO_THROW(instance.checkConfigFile()); } +// Verify write config file checker. +TEST_F(DaemonTest, checkWriteConfigFile) { + Daemon instance; + + std::string file(""); + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "/tmp/"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "tmp/"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + instance.setConfigFile("/tmp/foo"); + file = "/foo/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "/tmp/foo/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "/tmp/bar"; + EXPECT_NO_THROW(instance.checkWriteConfigFile(file)); + EXPECT_EQ("/tmp/bar", file); + file = "bar"; + EXPECT_NO_THROW(instance.checkWriteConfigFile(file)); + EXPECT_EQ("/tmp/bar", file); + instance.setConfigFile("tmp/foo"); + file = "/tmp/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "/tmp/foo/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "tmp/bar"; + EXPECT_NO_THROW(instance.checkWriteConfigFile(file)); + EXPECT_EQ("tmp/bar", file); + instance.setConfigFile("foo"); + file = "/tmp/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "tmp/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "foo/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "bar"; + EXPECT_NO_THROW(instance.checkWriteConfigFile(file)); + EXPECT_EQ("bar", file); +} + // Verify process name accessors TEST_F(DaemonTest, getSetProcName) { Daemon instance; @@ -321,7 +361,6 @@ TEST_F(DaemonTest, exitValue) { EXPECT_EQ(77, instance.getExitValue()); } - // More tests will appear here as we develop Daemon class. -}; +} diff --git a/src/lib/util/filesystem.cc b/src/lib/util/filesystem.cc index 8a752ca577..15db19efa8 100644 --- a/src/lib/util/filesystem.cc +++ b/src/lib/util/filesystem.cc @@ -90,11 +90,14 @@ setUmask() { } Path::Path(string const& full_name) { + dir_present_ = false; if (!full_name.empty()) { - bool dir_present = false; // Find the directory. size_t last_slash = full_name.find_last_of('/'); if (last_slash != string::npos) { + // Found a directory so note the fact. + dir_present_ = true; + // Found the last slash, so extract directory component and // set where the scan for the last_dot should terminate. parent_path_ = full_name.substr(0, last_slash); @@ -103,14 +106,11 @@ Path::Path(string const& full_name) { // do any more searching. return; } - - // Found a directory so note the fact. - dir_present = true; } // Now search backwards for the last ".". size_t last_dot = full_name.find_last_of('.'); - if ((last_dot == string::npos) || (dir_present && (last_dot < last_slash))) { + if ((last_dot == string::npos) || (dir_present_ && (last_dot < last_slash))) { // Last "." either not found or it occurs to the left of the last // slash if a directory was present (so it is part of a directory // name). In this case, the remainder of the string after the slash @@ -132,7 +132,7 @@ Path::Path(string const& full_name) { string Path::str() const { - return (parent_path_ + ((parent_path_.empty() || parent_path_ == "/") ? string() : "/") + stem_ + extension_); + return (parent_path_ + (dir_present_ ? "/" : "") + stem_ + extension_); } string @@ -140,6 +140,11 @@ Path::parentPath() const { return (parent_path_); } +string +Path::parentDirectory() const { + return (parent_path_ + (dir_present_ ? "/" : "")); +} + string Path::stem() const { return (stem_); @@ -174,10 +179,9 @@ Path::replaceExtension(string const& replacement) { Path& Path::replaceParentPath(string const& replacement) { string const trimmed_replacement(trim(replacement)); - if (trimmed_replacement.empty()) { + dir_present_ = (trimmed_replacement.find_last_of('/') != string::npos); + if (trimmed_replacement.empty() || (trimmed_replacement == "/")) { parent_path_ = string(); - } else if (trimmed_replacement == "/") { - parent_path_ = trimmed_replacement; } else if (trimmed_replacement.at(trimmed_replacement.size() - 1) == '/') { parent_path_ = trimmed_replacement.substr(0, trimmed_replacement.size() - 1); } else { diff --git a/src/lib/util/filesystem.h b/src/lib/util/filesystem.h index 91e0f4b2d1..e3ed083795 100644 --- a/src/lib/util/filesystem.h +++ b/src/lib/util/filesystem.h @@ -83,6 +83,14 @@ struct Path { /// @return parent path of current path. std::string parentPath() const; + /// @brief Get the parent directory. + /// + /// Empty if no directory is present, the parent path follwed by + /// a slash otherwise. + /// + /// @return parent directory of current path. + std::string parentDirectory() const; + /// @brief Get the base name of the file without the extension. /// /// Counterpart for std::filesystem::path::stem. @@ -129,6 +137,9 @@ struct Path { Path& replaceParentPath(std::string const& replacement = std::string()); private: + /// @brief Is a directory present. + bool dir_present_; + /// @brief Parent path. std::string parent_path_; diff --git a/src/lib/util/tests/filesystem_unittests.cc b/src/lib/util/tests/filesystem_unittests.cc index f5829f4d52..af1346eeaa 100644 --- a/src/lib/util/tests/filesystem_unittests.cc +++ b/src/lib/util/tests/filesystem_unittests.cc @@ -111,9 +111,55 @@ TEST(PathTest, components) { Path fname("/alpha/beta/gamma.delta"); EXPECT_EQ("/alpha/beta/gamma.delta", fname.str()); EXPECT_EQ("/alpha/beta", fname.parentPath()); + EXPECT_EQ("/alpha/beta/", fname.parentDirectory()); EXPECT_EQ("gamma", fname.stem()); EXPECT_EQ(".delta", fname.extension()); EXPECT_EQ("gamma.delta", fname.filename()); + + // The root. + Path root("/"); + EXPECT_EQ("/", root.str()); + EXPECT_EQ("", root.parentPath()); + EXPECT_EQ("/", root.parentDirectory()); + EXPECT_EQ("", root.stem()); + EXPECT_EQ("", root.extension()); + EXPECT_EQ("", root.filename()); + + // In the root directory. + Path inroot("/gamma.delta"); + EXPECT_EQ("/gamma.delta", inroot.str()); + EXPECT_EQ("", inroot.parentPath()); + EXPECT_EQ("/", inroot.parentDirectory()); + EXPECT_EQ("gamma", inroot.stem()); + EXPECT_EQ(".delta", inroot.extension()); + EXPECT_EQ("gamma.delta", inroot.filename()); + + // No directory. + Path nodir("gamma.delta"); + EXPECT_EQ("gamma.delta", nodir.str()); + EXPECT_EQ("", nodir.parentPath()); + EXPECT_EQ("", nodir.parentDirectory()); + EXPECT_EQ("gamma", nodir.stem()); + EXPECT_EQ(".delta", nodir.extension()); + EXPECT_EQ("gamma.delta", nodir.filename()); + + // Relative name. + Path relative("../alpha/gamma.delta"); + EXPECT_EQ("../alpha/gamma.delta", relative.str()); + EXPECT_EQ("../alpha", relative.parentPath()); + EXPECT_EQ("../alpha/", relative.parentDirectory()); + EXPECT_EQ("gamma", relative.stem()); + EXPECT_EQ(".delta", relative.extension()); + EXPECT_EQ("gamma.delta", relative.filename()); + + // Multiple extensions. + Path extensions("/alpha/beta/gamma.delta.epsilon"); + EXPECT_EQ("/alpha/beta/gamma.delta.epsilon", extensions.str()); + EXPECT_EQ("/alpha/beta", extensions.parentPath()); + EXPECT_EQ("/alpha/beta/", extensions.parentDirectory()); + EXPECT_EQ("gamma.delta", extensions.stem()); + EXPECT_EQ(".epsilon", extensions.extension()); + EXPECT_EQ("gamma.delta.epsilon", extensions.filename()); } /// @brief Check replaceExtension. @@ -133,30 +179,37 @@ TEST(PathTest, replaceExtension) { TEST(PathTest, replaceParentPath) { Path fname("a.b"); EXPECT_EQ("", fname.parentPath()); + EXPECT_EQ("", fname.parentDirectory()); EXPECT_EQ("a.b", fname.str()); fname.replaceParentPath("/just/some/dir/"); EXPECT_EQ("/just/some/dir", fname.parentPath()); + EXPECT_EQ("/just/some/dir/", fname.parentDirectory()); EXPECT_EQ("/just/some/dir/a.b", fname.str()); fname.replaceParentPath("/just/some/dir"); EXPECT_EQ("/just/some/dir", fname.parentPath()); + EXPECT_EQ("/just/some/dir/", fname.parentDirectory()); EXPECT_EQ("/just/some/dir/a.b", fname.str()); fname.replaceParentPath("/"); - EXPECT_EQ("/", fname.parentPath()); + EXPECT_EQ("", fname.parentPath()); + EXPECT_EQ("/", fname.parentDirectory()); EXPECT_EQ("/a.b", fname.str()); fname.replaceParentPath(""); EXPECT_EQ("", fname.parentPath()); + EXPECT_EQ("", fname.parentDirectory()); EXPECT_EQ("a.b", fname.str()); fname = Path("/first/a.b"); EXPECT_EQ("/first", fname.parentPath()); + EXPECT_EQ("/first/", fname.parentDirectory()); EXPECT_EQ("/first/a.b", fname.str()); fname.replaceParentPath("/just/some/dir"); EXPECT_EQ("/just/some/dir", fname.parentPath()); + EXPECT_EQ("/just/some/dir/", fname.parentDirectory()); EXPECT_EQ("/just/some/dir/a.b", fname.str()); }