diff --git a/common/SConscript b/common/SConscript index 542e10d43..a41fc8b7e 100644 --- a/common/SConscript +++ b/common/SConscript @@ -1,4 +1,4 @@ -Import('envCython') +Import('envCython', 'common') envCython.Program('clock.so', 'clock.pyx') -envCython.Program('params_pyx.so', 'params_pyx.pyx') +envCython.Program('params_pyx.so', 'params_pyx.pyx', LIBS=envCython['LIBS'] + [common, 'zmq']) diff --git a/selfdrive/common/params.cc b/selfdrive/common/params.cc index 0d7534554..c831cb3d8 100644 --- a/selfdrive/common/params.cc +++ b/selfdrive/common/params.cc @@ -10,12 +10,25 @@ #include #include #include - +#include #include #include #include "common/util.h" +#include "common/swaglog.h" +// keep trying if x gets interrupted by a signal +#define HANDLE_EINTR(x) \ + ({ \ + decltype(x) ret; \ + int try_cnt = 0; \ + do { \ + ret = (x); \ + } while (ret == -1 && errno == EINTR && try_cnt++ < 100); \ + ret; \ + }) + +namespace { #if defined(QCOM) || defined(QCOM2) const std::string default_params_path = "/data/params"; @@ -35,8 +48,8 @@ void params_sig_handler(int signal) { params_do_exit = 1; } -static int fsync_dir(const char* path){ - int fd = open(path, O_RDONLY, 0755); +int fsync_dir(const char* path){ + int fd = HANDLE_EINTR(open(path, O_RDONLY, 0755)); if (fd < 0){ return -1; } @@ -50,7 +63,7 @@ static int fsync_dir(const char* path){ } // TODO: replace by std::filesystem::create_directories -static int mkdir_p(std::string path) { +int mkdir_p(std::string path) { char * _path = (char *)path.c_str(); mode_t prev_mask = umask(0); @@ -71,7 +84,7 @@ static int mkdir_p(std::string path) { return 0; } -static bool ensure_params_path(const std::string ¶m_path, const std::string &key_path) { +bool ensure_params_path(const std::string ¶m_path, const std::string &key_path) { // Make sure params path exists if (!util::file_exists(param_path) && mkdir_p(param_path) != 0) { return false; @@ -110,6 +123,31 @@ static bool ensure_params_path(const std::string ¶m_path, const std::string return chmod(key_path.c_str(), 0777) == 0; } +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) { + close(fd_); + LOGE("Failed to lock file %s, errno=%d", fn_.c_str(), errno); + } + } + + void unlock() { close(fd_); } + +private: + int fd_ = -1, op_; + std::string fn_; +}; + +} // namespace + Params::Params(bool persistent_param) : Params(persistent_param ? persistent_params_path : default_params_path) {} Params::Params(const std::string &path) : params_path(path) { @@ -126,110 +164,54 @@ int Params::put(const char* key, const char* value, size_t value_size) { // 4) rename the temp file to the real name // 5) fsync() the containing directory - int lock_fd = -1; - int tmp_fd = -1; - int result; - std::string path; - std::string tmp_path; - ssize_t bytes_written; + std::string tmp_path = params_path + "/.tmp_value_XXXXXX"; + int tmp_fd = mkstemp((char*)tmp_path.c_str()); + if (tmp_fd < 0) return -1; - // Write value to temp. - tmp_path = params_path + "/.tmp_value_XXXXXX"; - tmp_fd = mkstemp((char*)tmp_path.c_str()); - bytes_written = write(tmp_fd, value, value_size); - if (bytes_written < 0 || (size_t)bytes_written != value_size) { - result = -20; - goto cleanup; - } - - // Build lock path - path = params_path + "/.lock"; - lock_fd = open(path.c_str(), O_CREAT, 0775); - - // Build key path - path = params_path + "/d/" + std::string(key); - - // Take lock. - result = flock(lock_fd, LOCK_EX); - if (result < 0) { - goto cleanup; - } - - // change permissions to 0666 for apks - result = fchmod(tmp_fd, 0666); - if (result < 0) { - goto cleanup; - } - - // fsync to force persist the changes. - result = fsync(tmp_fd); - if (result < 0) { - goto cleanup; - } - - // Move temp into place. - result = rename(tmp_path.c_str(), path.c_str()); - if (result < 0) { - goto cleanup; - } - - // fsync parent directory - path = params_path + "/d"; - result = fsync_dir(path.c_str()); - if (result < 0) { - goto cleanup; - } - -cleanup: - // Release lock. - if (lock_fd >= 0) { - close(lock_fd); - } - if (tmp_fd >= 0) { - if (result < 0) { - remove(tmp_path.c_str()); + int result = -1; + do { + // Write value to temp. + ssize_t bytes_written = HANDLE_EINTR(write(tmp_fd, value, value_size)); + if (bytes_written < 0 || (size_t)bytes_written != value_size) { + result = -20; + break; } - close(tmp_fd); - } + + // change permissions to 0666 for apks + if ((result = fchmod(tmp_fd, 0666)) < 0) break; + // fsync to force persist the changes. + if ((result = fsync(tmp_fd)) < 0) break; + + FileLock file_lock(params_path + "/.lock", LOCK_EX); + std::lock_guard lk(file_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; + + // fsync parent directory + path = params_path + "/d"; + result = fsync_dir(path.c_str()); + } while(0); + + close(tmp_fd); + remove(tmp_path.c_str()); return result; } int Params::remove(const char *key) { - int lock_fd = -1; - int result; - std::string path; - - // Build lock path, and open lockfile - path = params_path + "/.lock"; - lock_fd = open(path.c_str(), O_CREAT, 0775); - - // Take lock. - result = flock(lock_fd, LOCK_EX); - if (result < 0) { - goto cleanup; - } - + FileLock file_lock(params_path + "/.lock", LOCK_EX); + std::lock_guard lk(file_lock); // Delete value. - path = params_path + "/d/" + key; - result = ::remove(path.c_str()); + std::string path = params_path + "/d/" + key; + int result = ::remove(path.c_str()); if (result != 0) { result = ERR_NO_VALUE; - goto cleanup; + return result; } - // fsync parent directory path = params_path + "/d"; - result = fsync_dir(path.c_str()); - if (result < 0) { - goto cleanup; - } - -cleanup: - // Release lock. - if (lock_fd >= 0) { - close(lock_fd); - } - return result; + return fsync_dir(path.c_str()); } std::string Params::get(const char *key, bool block) { @@ -257,37 +239,20 @@ std::string Params::get(const char *key, bool block) { } int Params::read_db_all(std::map *params) { - int err = 0; - - std::string lock_path = params_path + "/.lock"; - - int lock_fd = open(lock_path.c_str(), 0); - if (lock_fd < 0) return -1; - - err = flock(lock_fd, LOCK_SH); - if (err < 0) { - close(lock_fd); - return err; - } + FileLock file_lock(params_path + "/.lock", LOCK_SH); + std::lock_guard lk(file_lock); std::string key_path = params_path + "/d"; DIR *d = opendir(key_path.c_str()); - if (!d) { - close(lock_fd); - return -1; - } + if (!d) return -1; struct dirent *de = NULL; while ((de = readdir(d))) { - if (!isalnum(de->d_name[0])) continue; - std::string key = std::string(de->d_name); - std::string value = util::read_file(key_path + "/" + key); - - (*params)[key] = value; + if (isalnum(de->d_name[0])) { + (*params)[de->d_name] = util::read_file(key_path + "/" + de->d_name); + } } closedir(d); - - close(lock_fd); return 0; }