params: code cleanup (#22744)

* cleanup params

* apply review

* continue

* use c_str

* cleanup filelock

* don't check return code of close()

* remove call_once

* cleanup params_pyx

* cleanup comment
old-commit-hash: 2773ff5aceb8874a9319c819ec3707705a00604d

common/params_pyx.pyx

@@ -14,7 +14,6 @@ cdef extern from "selfdrive/common/params.h":
     ALL
 
   cdef cppclass c_Params "Params":
-    c_Params() nogil
     c_Params(string) nogil
     string get(string, bool) nogil
     bool getBool(string) nogil
@@ -34,36 +33,25 @@ class UnknownKeyName(Exception):
 cdef class Params:
   cdef c_Params* p
 
-  def __cinit__(self, d=None):
-    cdef string path
-    if d is None:
-      with nogil:
-        self.p = new c_Params()
-    else:
-      path = <string>d.encode()
-      with nogil:
-        self.p = new c_Params(path)
+  def __cinit__(self, d=""):
+    cdef string path = <string>d.encode()
+    with nogil:
+      self.p = new c_Params(path)
 
   def __dealloc__(self):
     del self.p
 
-  def clear_all(self, tx_type=None):
-    if tx_type is None:
-      tx_type = ParamKeyType.ALL
-
+  def clear_all(self, tx_type=ParamKeyType.ALL):
     self.p.clearAll(tx_type)
 
   def check_key(self, key):
     key = ensure_bytes(key)
-
     if not self.p.checkKey(key):
       raise UnknownKeyName(key)
-
     return key
 
   def get(self, key, bool block=False, encoding=None):
     cdef string k = self.check_key(key)
-
     cdef string val
     with nogil:
       val = self.p.get(k, block)
@@ -76,10 +64,7 @@ cdef class Params:
       else:
         return None
 
-    if encoding is not None:
-      return val.decode(encoding)
-    else:
-      return val
+    return val if encoding is None else val.decode(encoding)
 
   def get_bool(self, key):
     cdef string k = self.check_key(key)
@@ -110,8 +95,7 @@ cdef class Params:
     with nogil:
       self.p.remove(k)
 
-
-def put_nonblocking(key, val, d=None):
+def put_nonblocking(key, val, d=""):
   def f(key, val):
     params = Params(d)
     cdef string k = ensure_bytes(key)

selfdrive/common/params.cc

@@ -1,19 +1,9 @@
 #include "selfdrive/common/params.h"
 
-#ifndef _GNU_SOURCE
-#define _GNU_SOURCE
-#endif  // _GNU_SOURCE
-
 #include <dirent.h>
 #include <sys/file.h>
-#include <sys/stat.h>
-#include <unistd.h>
 
 #include <csignal>
-#include <cstdio>
-#include <cstdlib>
-#include <cstring>
-#include <mutex>
 #include <unordered_map>
 
 #include "selfdrive/common/swaglog.h"
@@ -27,21 +17,16 @@ void params_sig_handler(int signal) {
   params_do_exit = 1;
 }
 
-int fsync_dir(const char* path) {
-  int fd = HANDLE_EINTR(open(path, O_RDONLY, 0755));
-  if (fd < 0) {
-    return -1;
-  }
-
-  int result = fsync(fd);
-  int result_close = close(fd);
-  if (result_close < 0) {
-    result = result_close;
+int fsync_dir(const std::string &path) {
+  int result = -1;
+  int fd = HANDLE_EINTR(open(path.c_str(), O_RDONLY, 0755));
+  if (fd >= 0) {
+    result = fsync(fd);
+    close(fd);
   }
   return result;
 }
 
-
 bool create_params_path(const std::string &param_path, const std::string &key_path) {
   // Make sure params path exists
   if (!util::file_exists(param_path) && !util::create_directories(param_path, 0775)) {
@@ -51,9 +36,8 @@ bool create_params_path(const std::string &param_path, const std::string &key_pa
   // See if the symlink exists, otherwise create it
   if (!util::file_exists(key_path)) {
     // 1) Create temp folder
-    // 2) Set permissions
-    // 3) Symlink it to temp link
-    // 4) Move symlink to <params>/d
+    // 2) Symlink it to temp link
+    // 3) Move symlink to <params>/d
 
     std::string tmp_path = param_path + "/.tmp_XXXXXX";
     // this should be OK since mkdtemp just replaces characters in place
@@ -76,32 +60,26 @@ bool create_params_path(const std::string &param_path, const std::string &key_pa
   return true;
 }
 
-void ensure_params_path(const std::string &params_path) {
+std::string ensure_params_path(const std::string &path = {}) {
+  std::string params_path = path.empty() ? Path::params() : path;
   if (!create_params_path(params_path, params_path + "/d")) {
     throw std::runtime_error(util::string_format("Failed to ensure params path, errno=%d", errno));
   }
+  return params_path;
 }
 
 class FileLock {
- public:
-  FileLock(const std::string& file_name, int op) : fn_(file_name), op_(op) {}
-
-  void lock() {
-    fd_ = HANDLE_EINTR(open(fn_.c_str(), O_CREAT, 0775));
-    if (fd_ < 0) {
-      LOGE("Failed to open lock file %s, errno=%d", fn_.c_str(), errno);
-      return;
-    }
-    if (HANDLE_EINTR(flock(fd_, op_)) < 0) {
-      LOGE("Failed to lock file %s, errno=%d", fn_.c_str(), errno);
+public:
+  FileLock(const std::string &fn) {
+    fd_ = HANDLE_EINTR(open(fn.c_str(), O_CREAT, 0775));
+    if (fd_ < 0 || HANDLE_EINTR(flock(fd_, LOCK_EX)) < 0) {
+      LOGE("Failed to lock file %s, errno=%d", fn.c_str(), errno);
     }
   }
-
-  void unlock() { close(fd_); }
+  ~FileLock() { close(fd_); }
 
 private:
-  int fd_ = -1, op_;
-  std::string fn_;
+  int fd_ = -1;
 };
 
 std::unordered_map<std::string, uint32_t> keys = {
@@ -192,13 +170,9 @@ std::unordered_map<std::string, uint32_t> keys = {
 
 } // namespace
 
-Params::Params() : params_path(Path::params()) {
-  static std::once_flag once_flag;
-  std::call_once(once_flag, ensure_params_path, params_path);
-}
-
-Params::Params(const std::string &path) : params_path(path) {
-  ensure_params_path(params_path);
+Params::Params(const std::string &path) {
+  static std::string default_param_path = ensure_params_path();
+  params_path = path.empty() ? default_param_path : ensure_params_path(path);
 }
 
 bool Params::checkKey(const std::string &key) {
@@ -232,16 +206,13 @@ int Params::put(const char* key, const char* value, size_t value_size) {
     // fsync to force persist the changes.
     if ((result = fsync(tmp_fd)) < 0) break;
 
-    FileLock file_lock(params_path + "/.lock", LOCK_EX);
-    std::lock_guard<FileLock> lk(file_lock);
+    FileLock file_lock(params_path + "/.lock");
 
     // Move temp into place.
-    std::string path = params_path + "/d/" + std::string(key);
-    if ((result = rename(tmp_path.c_str(), path.c_str())) < 0) break;
+    if ((result = rename(tmp_path.c_str(), getParamPath(key).c_str())) < 0) break;
 
     // fsync parent directory
-    path = params_path + "/d";
-    result = fsync_dir(path.c_str());
+    result = fsync_dir(getParamPath());
   } while (false);
 
   close(tmp_fd);
@@ -249,24 +220,18 @@ int Params::put(const char* key, const char* value, size_t value_size) {
   return result;
 }
 
-int Params::remove(const char *key) {
-  FileLock file_lock(params_path + "/.lock", LOCK_EX);
-  std::lock_guard<FileLock> lk(file_lock);
-  // Delete value.
-  std::string path = params_path + "/d/" + key;
-  int result = unlink(path.c_str());
+int Params::remove(const std::string &key) {
+  FileLock file_lock(params_path + "/.lock");
+  int result = unlink(getParamPath(key).c_str());
   if (result != 0) {
     return result;
   }
-  // fsync parent directory
-  path = params_path + "/d";
-  return fsync_dir(path.c_str());
+  return fsync_dir(getParamPath());
 }
 
-std::string Params::get(const char *key, bool block) {
-  std::string path = params_path + "/d/" + key;
+std::string Params::get(const std::string &key, bool block) {
   if (!block) {
-    return util::read_file(path);
+    return util::read_file(getParamPath(key));
   } else {
     // blocking read until successful
     params_do_exit = 0;
@@ -275,7 +240,7 @@ std::string Params::get(const char *key, bool block) {
 
     std::string value;
     while (!params_do_exit) {
-      if (value = util::read_file(path); !value.empty()) {
+      if (value = util::read_file(getParamPath(key)); !value.empty()) {
         break;
       }
       util::sleep_for(100);  // 0.1 s
@@ -288,26 +253,19 @@ std::string Params::get(const char *key, bool block) {
 }
 
 std::map<std::string, std::string> Params::readAll() {
-  FileLock file_lock(params_path + "/.lock", LOCK_SH);
-  std::lock_guard<FileLock> lk(file_lock);
-
-  std::string key_path = params_path + "/d";
-  return util::read_files_in_dir(key_path);
+  FileLock file_lock(params_path + "/.lock");
+  return util::read_files_in_dir(getParamPath());
 }
 
 void Params::clearAll(ParamKeyType key_type) {
-  FileLock file_lock(params_path + "/.lock", LOCK_EX);
-  std::lock_guard<FileLock> lk(file_lock);
+  FileLock file_lock(params_path + "/.lock");
 
   std::string path;
   for (auto &[key, type] : keys) {
     if (type & key_type) {
-      path = params_path + "/d/" + key;
-      unlink(path.c_str());
+      unlink(getParamPath(key).c_str());
     }
   }
 
-  // fsync parent directory
-  path = params_path + "/d";
-  fsync_dir(path.c_str());
+  fsync_dir(getParamPath());
 }

selfdrive/common/params.h

@@ -1,9 +1,7 @@
 #pragma once
 
 #include <map>
-#include <sstream>
 #include <string>
-#include <optional>
 
 enum ParamKeyType {
   PERSISTENT = 0x02,
@@ -17,68 +15,33 @@ enum ParamKeyType {
 
 class Params {
 public:
-  Params();
-  Params(const std::string &path);
-
+  Params(const std::string &path = {});
   bool checkKey(const std::string &key);
   ParamKeyType getKeyType(const std::string &key);
+  inline std::string getParamPath(const std::string &key = {}) {
+    return key.empty() ? params_path + "/d" : params_path + "/d/" + key;
+  }
 
   // Delete a value
-  int remove(const char *key);
-  inline int remove(const std::string &key) {
-    return remove (key.c_str());
-  }
+  int remove(const std::string &key);
   void clearAll(ParamKeyType type);
 
-  // read all values
-  std::map<std::string, std::string> readAll();
-
   // helpers for reading values
-  std::string get(const char *key, bool block = false);
-
-  inline std::string get(const std::string &key, bool block = false) {
-    return get(key.c_str(), block);
-  }
-
-  inline std::string getParamsPath() {
-    return params_path;
-  }
-
-  inline std::string getParamPath(std::string key) {
-    return params_path + "/d/" + key;
-  }
-
-  template <class T>
-  std::optional<T> get(const char *key, bool block = false) {
-    std::istringstream iss(get(key, block));
-    T value{};
-    iss >> value;
-    return iss.fail() ? std::nullopt : std::optional(value);
-  }
-
+  std::string get(const std::string &key, bool block = false);
   inline bool getBool(const std::string &key) {
-    return getBool(key.c_str());
-  }
-
-  inline bool getBool(const char *key) {
     return get(key) == "1";
   }
+  std::map<std::string, std::string> readAll();
 
   // helpers for writing values
-  int put(const char* key, const char* val, size_t value_size);
-
+  int put(const char *key, const char *val, size_t value_size);
   inline int put(const std::string &key, const std::string &val) {
     return put(key.c_str(), val.data(), val.size());
   }
-
-  inline int putBool(const char *key, bool val) {
-    return put(key, val ? "1" : "0", 1);
-  }
-
   inline int putBool(const std::string &key, bool val) {
-    return putBool(key.c_str(), val);
+    return put(key.c_str(), val ? "1" : "0", 1);
   }
 
 private:
-  const std::string params_path;
+  std::string params_path;
 };

selfdrive/ui/qt/offroad/settings.cc

@@ -227,8 +227,7 @@ SoftwarePanel::SoftwarePanel(QWidget* parent) : ListWidget(parent) {
 
   fs_watch = new QFileSystemWatcher(this);
   QObject::connect(fs_watch, &QFileSystemWatcher::fileChanged, [=](const QString path) {
-    int update_failed_count = params.get<int>("UpdateFailedCount").value_or(0);
-    if (path.contains("UpdateFailedCount") && update_failed_count > 0) {
+    if (path.contains("UpdateFailedCount") && std::atoi(params.get("UpdateFailedCount").c_str()) > 0) {
       lastUpdateLbl->setText("failed to fetch update");
       updateBtn->setText("CHECK");
       updateBtn->setEnabled(true);