openpilot v0.3.7 release

old-commit-hash: daf54ad54d

common/params.py

@@ -66,6 +66,13 @@ keys = {
 # read:    radard
   "CarParams": TxType.CLEAR_ON_CAR_START}
 
+def fsync_dir(path):
+  fd = os.open(path, os.O_RDONLY)
+  try:
+    os.fsync(fd)
+  finally:
+    os.close(fd)
+
 
 class FileLock(object):
   def __init__(self, path, create):
@@ -182,15 +189,32 @@ class DBWriter(DBAccessor):
     self._check_entered()
 
     try:
+      # data_path refers to the externally used path to the params. It is a symlink.
+      # old_data_path is the path currently pointed to by data_path.
+      # tempdir_path is a path where the new params will go, which the new data path will point to.
+      # new_data_path is a temporary symlink that will atomically overwrite data_path.
+      #
+      # The current situation is:
+      #   data_path -> old_data_path
+      # We're going to write params data to tempdir_path
+      #   tempdir_path -> params data
+      # Then point new_data_path to tempdir_path
+      #   new_data_path -> tempdir_path
+      # Then atomically overwrite data_path with new_data_path
+      #   data_path -> tempdir_path
       old_data_path = None
       new_data_path = None
       tempdir_path = tempfile.mkdtemp(prefix=".tmp", dir=self._path)
+
       try:
         # Write back all keys.
         os.chmod(tempdir_path, 0o777)
         for k, v in self._vals.items():
           with open(os.path.join(tempdir_path, k), "wb") as f:
             f.write(v)
+            f.flush()
+            os.fsync(f.fileno())
+        fsync_dir(tempdir_path)
 
         data_path = self._data_path()
         try:
@@ -203,16 +227,21 @@ class DBWriter(DBAccessor):
         new_data_path = "{}.link".format(tempdir_path)
         os.symlink(os.path.basename(tempdir_path), new_data_path)
         os.rename(new_data_path, data_path)
-      # TODO(mgraczyk): raise useful error when values are bad.
-      except:
-        shutil.rmtree(tempdir_path)
-        if new_data_path is not None:
-          os.remove(new_data_path)
-        raise
+        fsync_dir(self._path)
+      finally:
+        # If the rename worked, we can delete the old data. Otherwise delete the new one.
+        success = new_data_path is not None and os.path.exists(data_path) and (
+          os.readlink(data_path) == os.path.basename(tempdir_path))
 
-      # Keep holding the lock while we clean up the old data.
-      if old_data_path is not None:
-        shutil.rmtree(old_data_path)
+        if success:
+          if old_data_path is not None:
+            shutil.rmtree(old_data_path)
+        else:
+          shutil.rmtree(tempdir_path)
+
+        # Regardless of what happened above, there should be no link at new_data_path.
+        if new_data_path is not None and os.path.islink(new_data_path):
+          os.remove(new_data_path)
     finally:
       os.umask(self._prev_umask)
       self._prev_umask = None
@@ -249,6 +278,10 @@ class Params(object):
   def car_start(self):
     self._clear_keys_with_type(TxType.CLEAR_ON_CAR_START)
 
+  def delete(self, key):
+    with self.env.begin(write=True) as txn:
+      txn.delete(key)
+
   def get(self, key, block=False):
     if key not in keys:
       raise UnknownKeyName(key)