[#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 64492a3c60e31a13b3f0d67321862c7a690194ee..64814bf01041c119113ba7372a2a2ab82924a2e0 100644 (file)
--- 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 0260d2688647ed015a22b8eccac2296da85a1d32..cae59227e63e64e97eac9d88499b6351ab2fface 100644 (file)
--- 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 e5d517979fa8c0f51a10af15b702f7e77ad65815..cba9ded080d01b4df20f3ddb32ac93ad106b3f9e 100644 (file)
--- 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 3430e21b4a2b0f97a59e1f03bf2a12e3b1cc3d77..0cfbf4c7768148c5259ab826a5f51b97e3e7be1e 100644 (file)
--- 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 8a752ca57737fdd42518f54323b0a7eed1a9d224..15db19efa8219e0d27c0be5cd15e3e9ee4922d3e 100644 (file)
--- 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 91e0f4b2d1d71c1561a49d42ee4051be819928b9..e3ed083795167241e66b66555a71e5a279430b67 100644 (file)
--- 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 f5829f4d522b2379167f8806db6ee672c42037d2..af1346eeaa2fd21d8f029f253f1e7aafb4bcefb5 100644 (file)
--- 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());
 }