sadmen
/
openpilot_comma
mirror of https://github.com/commaai/openpilot.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

params: code cleanup (#22744)

Browse Source 
* 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
pull/22769/head
Dean Lee 4 years ago committed by GitHub
parent ea9b93008e
commit 2773ff5ace
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 54 additions and 150 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Unified View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 30
      common/params_pyx.pyx
  2. 114
      selfdrive/common/params.cc
  3. 57
      selfdrive/common/params.h
  4. 3
      selfdrive/ui/qt/offroad/settings.cc

30
common/params_pyx.pyx
Unescape View File

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

114
selfdrive/common/params.cc
Unescape View File

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

57
selfdrive/common/params.h
Unescape View File

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

3
selfdrive/ui/qt/offroad/settings.cc
Unescape View File

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

Write Preview
Loading…
Cancel
Save