From 10a1b8b7b172420047fab5caa84829dd510e1e38 Mon Sep 17 00:00:00 2001
From: Dean Lee <deanlee3@gmail.com>
Date: Fri, 5 May 2023 02:21:41 +0800
Subject: [PATCH] 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            | 17 +++++++++++------
 common/tests/test_params.py |  9 +++++++++
 common/tests/test_util.cc   | 17 -----------------
 common/util.cc              | 16 ----------------
 common/util.h               |  1 -
 5 files changed, 20 insertions(+), 40 deletions(-)

diff --git a/common/params.cc b/common/params.cc
index 428830a112..a5e6381d9c 100644
--- a/common/params.cc
+++ b/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) {
-    util::remove_files_in_dir(getParamPath());
-  } else {
-    for (auto &[key, type] : keys) {
-      if (type & key_type) {
-        unlink(getParamPath(key).c_str());
+  // 1) delete params of key_type
+  // 2) delete files that are not defined in the keys.
+  if (DIR *d = opendir(getParamPath().c_str())) {
+    struct dirent *de = NULL;
+    while ((de = readdir(d))) {
+      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());
diff --git a/common/tests/test_params.py b/common/tests/test_params.py
index ec13e82217..d432218c8a 100644
--- a/common/tests/test_params.py
+++ b/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")
diff --git a/common/tests/test_util.cc b/common/tests/test_util.cc
index b70cc9044a..25ecf09aa9 100644
--- a/common/tests/test_util.cc
+++ b/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());
-}
diff --git a/common/util.cc b/common/util.cc
index 10dff6a9ea..ee1080cbdf 100644
--- a/common/util.cc
+++ b/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) {
diff --git a/common/util.h b/common/util.h
index 028074384e..c80dc234f2 100644
--- a/common/util.h
+++ b/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);