params: cleanup old params that aren't defined (#28017)

* delete files that are not defined in the keys

* assert after create file

* remove util::remove_files_in_dir

* cleanup

* fix up test

---------

Co-authored-by: Adeeb Shihadeh <adeebshihadeh@gmail.com>

common/params.cc

@@ -305,14 +305,19 @@ std::map<std::string, std::string> Params::readAll() {
 void Params::clearAll(ParamKeyType key_type) {
   FileLock file_lock(params_path + "/.lock");
 
-  if (key_type == ALL) {
+  // 1) delete params of key_type
-    util::remove_files_in_dir(getParamPath());
+  // 2) delete files that are not defined in the keys.
-  } else {
+  if (DIR *d = opendir(getParamPath().c_str())) {
-    for (auto &[key, type] : keys) {
+    struct dirent *de = NULL;
-      if (type & key_type) {
+    while ((de = readdir(d))) {
-        unlink(getParamPath(key).c_str());
+      if (de->d_type != DT_DIR) {
+        auto it = keys.find(de->d_name);
+        if (it == keys.end() || (it->second & key_type)) {
+          unlink(getParamPath(de->d_name).c_str());
+        }
       }
     }
+    closedir(d);
   }
 
   fsync_dir(getParamPath());

common/tests/test_params.py

@@ -1,7 +1,9 @@
+import os
 import threading
 import time
 import tempfile
 import shutil
+import uuid
 import unittest
 
 from common.params import Params, ParamKeyType, UnknownKeyName, put_nonblocking, put_bool_nonblocking
@@ -28,9 +30,16 @@ class TestParams(unittest.TestCase):
     self.params.put("CarParams", "test")
     self.params.put("DongleId", "cb38263377b873ee")
     assert self.params.get("CarParams") == b"test"
+
+    undefined_param = self.params.get_param_path(uuid.uuid4().hex)
+    with open(undefined_param, "w") as f:
+      f.write("test")
+    assert os.path.isfile(undefined_param)
+
     self.params.clear_all(ParamKeyType.CLEAR_ON_MANAGER_START)
     assert self.params.get("CarParams") is None
     assert self.params.get("DongleId") is not None
+    assert not os.path.isfile(undefined_param)
 
   def test_params_two_things(self):
     self.params.put("DongleId", "bob")

common/tests/test_util.cc

@@ -143,20 +143,3 @@ TEST_CASE("util::create_directories") {
     REQUIRE(util::create_directories("", 0755) == false);
   }
 }
-
-TEST_CASE("util::remove_files_in_dir") {
-  std::string tmp_dir = "/tmp/test_remove_all_in_dir";
-  system("rm /tmp/test_remove_all_in_dir -rf");
-  REQUIRE(util::create_directories(tmp_dir, 0755));
-  const int tmp_file_cnt = 10;
-  for (int i = 0; i < tmp_file_cnt; ++i) {
-    std::string tmp_file = tmp_dir + "/test_XXXXXX";
-    int fd = mkstemp((char*)tmp_file.c_str());
-    close(fd);
-    REQUIRE(util::file_exists(tmp_file.c_str()));
-  }
-
-  REQUIRE(util::read_files_in_dir(tmp_dir).size() == tmp_file_cnt);
-  util::remove_files_in_dir(tmp_dir);
-  REQUIRE(util::read_files_in_dir(tmp_dir).empty());
-}

common/util.cc

@@ -99,22 +99,6 @@ std::map<std::string, std::string> read_files_in_dir(const std::string &path) {
   return ret;
 }
 
-void remove_files_in_dir(const std::string &path) {
-  DIR *d = opendir(path.c_str());
-  if (!d) return;
-
-  std::string fn;
-  struct dirent *de = NULL;
-  while ((de = readdir(d))) {
-    if (de->d_type != DT_DIR) {
-      fn = path + "/" + de->d_name;
-      unlink(fn.c_str());
-    }
-  }
-
-  closedir(d);
-}
-
 int write_file(const char* path, const void* data, size_t size, int flags, mode_t mode) {
   int fd = HANDLE_EINTR(open(path, flags, mode));
   if (fd == -1) {

common/util.h

@@ -81,7 +81,6 @@ std::string dir_name(std::string const& path);
 // **** file fhelpers *****
 std::string read_file(const std::string& fn);
 std::map<std::string, std::string> read_files_in_dir(const std::string& path);
-void remove_files_in_dir(const std::string& path);
 int write_file(const char* path, const void* data, size_t size, int flags = O_WRONLY, mode_t mode = 0664);
 
 FILE* safe_fopen(const char* filename, const char* mode);