From 81dacbedcacaf9db43d19700e04e7361c0fbbbcc Mon Sep 17 00:00:00 2001 From: Adeeb Shihadeh Date: Sun, 15 May 2022 17:13:50 -0700 Subject: [PATCH] common: replace custom xattr implementation with os module's (#24448) * use os module's xattr function * fix that * handle in helper --- common/tests/test_xattr.py | 47 -------------------- release/files_common | 1 - selfdrive/loggerd/tests/test_deleter.py | 4 +- selfdrive/loggerd/tests/test_uploader.py | 13 +++--- selfdrive/loggerd/tools/mark_all_uploaded.py | 3 +- selfdrive/loggerd/tools/mark_unuploaded.py | 4 +- selfdrive/loggerd/xattr_cache.py | 30 ++++++++----- 7 files changed, 30 insertions(+), 72 deletions(-) delete mode 100644 common/tests/test_xattr.py mode change 100644 => 100755 selfdrive/loggerd/tests/test_deleter.py diff --git a/common/tests/test_xattr.py b/common/tests/test_xattr.py deleted file mode 100644 index 16f6d52977..0000000000 --- a/common/tests/test_xattr.py +++ /dev/null @@ -1,47 +0,0 @@ -import os -import tempfile -import shutil -import unittest - -from common.xattr import getxattr, setxattr, listxattr, removexattr - -class TestParams(unittest.TestCase): - USER_TEST='user.test' - def setUp(self): - self.tmpdir = tempfile.mkdtemp() - self.tmpfn = os.path.join(self.tmpdir, 'test.txt') - open(self.tmpfn, 'w').close() - #print("using", self.tmpfn) - - def tearDown(self): - shutil.rmtree(self.tmpdir) - - def test_getxattr_none(self): - a = getxattr(self.tmpfn, TestParams.USER_TEST) - assert a is None - - def test_listxattr_none(self): - l = listxattr(self.tmpfn) - assert l == [] - - def test_setxattr(self): - setxattr(self.tmpfn, TestParams.USER_TEST, b'123') - a = getxattr(self.tmpfn, TestParams.USER_TEST) - assert a == b'123' - - def test_listxattr(self): - setxattr(self.tmpfn, 'user.test1', b'123') - setxattr(self.tmpfn, 'user.test2', b'123') - l = listxattr(self.tmpfn) - assert l == ['user.test1', 'user.test2'] - - def test_removexattr(self): - setxattr(self.tmpfn, TestParams.USER_TEST, b'123') - a = getxattr(self.tmpfn, TestParams.USER_TEST) - assert a == b'123' - removexattr(self.tmpfn, TestParams.USER_TEST) - a = getxattr(self.tmpfn, TestParams.USER_TEST) - assert a is None - -if __name__ == "__main__": - unittest.main() diff --git a/release/files_common b/release/files_common index 7dfafcb26c..6c058e83b5 100644 --- a/release/files_common +++ b/release/files_common @@ -28,7 +28,6 @@ common/logging_extra.py common/numpy_fast.py common/params.py common/params_pyx.pyx -common/xattr.py common/profiler.py common/basedir.py common/dict_helpers.py diff --git a/selfdrive/loggerd/tests/test_deleter.py b/selfdrive/loggerd/tests/test_deleter.py old mode 100644 new mode 100755 index e0a063bd43..80fb5c997f --- a/selfdrive/loggerd/tests/test_deleter.py +++ b/selfdrive/loggerd/tests/test_deleter.py @@ -1,12 +1,12 @@ +#!/usr/bin/env python3 import os import time import threading import unittest from collections import namedtuple -import selfdrive.loggerd.deleter as deleter from common.timeout import Timeout, TimeoutException - +import selfdrive.loggerd.deleter as deleter from selfdrive.loggerd.tests.loggerd_tests_common import UploaderTestCase Stats = namedtuple("Stats", ['f_bavail', 'f_blocks', 'f_frsize']) diff --git a/selfdrive/loggerd/tests/test_uploader.py b/selfdrive/loggerd/tests/test_uploader.py index dd81108700..b8c01776ae 100755 --- a/selfdrive/loggerd/tests/test_uploader.py +++ b/selfdrive/loggerd/tests/test_uploader.py @@ -9,8 +9,6 @@ import json from selfdrive.swaglog import cloudlog import selfdrive.loggerd.uploader as uploader -from common.xattr import getxattr - from selfdrive.loggerd.tests.loggerd_tests_common import UploaderTestCase @@ -84,7 +82,7 @@ class TestUploader(UploaderTestCase): self.assertFalse(len(log_handler.upload_order) < len(exp_order), "Some files failed to upload") self.assertFalse(len(log_handler.upload_order) > len(exp_order), "Some files were uploaded twice") for f_path in exp_order: - self.assertTrue(getxattr(os.path.join(self.root, f_path.replace('.bz2', '')), uploader.UPLOAD_ATTR_NAME), "All files not uploaded") + self.assertTrue(os.getxattr(os.path.join(self.root, f_path.replace('.bz2', '')), uploader.UPLOAD_ATTR_NAME), "All files not uploaded") self.assertTrue(log_handler.upload_order == exp_order, "Files uploaded in wrong order") @@ -103,7 +101,7 @@ class TestUploader(UploaderTestCase): self.assertFalse(len(log_handler.upload_ignored) < len(exp_order), "Some files failed to ignore") self.assertFalse(len(log_handler.upload_ignored) > len(exp_order), "Some files were ignored twice") for f_path in exp_order: - self.assertTrue(getxattr(os.path.join(self.root, f_path.replace('.bz2', '')), uploader.UPLOAD_ATTR_NAME), "All files not ignored") + self.assertTrue(os.getxattr(os.path.join(self.root, f_path.replace('.bz2', '')), uploader.UPLOAD_ATTR_NAME), "All files not ignored") self.assertTrue(log_handler.upload_ignored == exp_order, "Files ignored in wrong order") @@ -128,7 +126,7 @@ class TestUploader(UploaderTestCase): self.assertFalse(len(log_handler.upload_order) < len(exp_order), "Some files failed to upload") self.assertFalse(len(log_handler.upload_order) > len(exp_order), "Some files were uploaded twice") for f_path in exp_order: - self.assertTrue(getxattr(os.path.join(self.root, f_path.replace('.bz2', '')), uploader.UPLOAD_ATTR_NAME), "All files not uploaded") + self.assertTrue(os.getxattr(os.path.join(self.root, f_path.replace('.bz2', '')), uploader.UPLOAD_ATTR_NAME), "All files not uploaded") self.assertTrue(log_handler.upload_order == exp_order, "Files uploaded in wrong order") @@ -143,7 +141,8 @@ class TestUploader(UploaderTestCase): self.join_thread() for f_path in f_paths: - self.assertFalse(getxattr(f_path.replace('.bz2', ''), uploader.UPLOAD_ATTR_NAME), "File upload when locked") + uploaded = uploader.UPLOAD_ATTR_NAME in os.listxattr(f_path.replace('.bz2', '')) + self.assertFalse(uploaded, "File upload when locked") def test_clear_locks_on_startup(self): f_paths = self.gen_files(lock=True, boot=False) @@ -156,4 +155,4 @@ class TestUploader(UploaderTestCase): if __name__ == "__main__": - unittest.main(failfast=True) + unittest.main() diff --git a/selfdrive/loggerd/tools/mark_all_uploaded.py b/selfdrive/loggerd/tools/mark_all_uploaded.py index ff6e3c1f17..e60e6cfa2c 100644 --- a/selfdrive/loggerd/tools/mark_all_uploaded.py +++ b/selfdrive/loggerd/tools/mark_all_uploaded.py @@ -1,9 +1,8 @@ import os -from common.xattr import setxattr from selfdrive.loggerd.uploader import UPLOAD_ATTR_NAME, UPLOAD_ATTR_VALUE from selfdrive.loggerd.config import ROOT for folder in os.walk(ROOT): for file1 in folder[2]: full_path = os.path.join(folder[0], file1) - setxattr(full_path, UPLOAD_ATTR_NAME, UPLOAD_ATTR_VALUE) + os.setxattr(full_path, UPLOAD_ATTR_NAME, UPLOAD_ATTR_VALUE) diff --git a/selfdrive/loggerd/tools/mark_unuploaded.py b/selfdrive/loggerd/tools/mark_unuploaded.py index 0d3f765af4..343805d5fc 100755 --- a/selfdrive/loggerd/tools/mark_unuploaded.py +++ b/selfdrive/loggerd/tools/mark_unuploaded.py @@ -1,8 +1,8 @@ #!/usr/bin/env python3 +import os import sys -from common.xattr import removexattr from selfdrive.loggerd.uploader import UPLOAD_ATTR_NAME for fn in sys.argv[1:]: print(f"unmarking {fn}") - removexattr(fn, UPLOAD_ATTR_NAME) + os.removexattr(fn, UPLOAD_ATTR_NAME) diff --git a/selfdrive/loggerd/xattr_cache.py b/selfdrive/loggerd/xattr_cache.py index d01bf1aa8a..95e39f2032 100644 --- a/selfdrive/loggerd/xattr_cache.py +++ b/selfdrive/loggerd/xattr_cache.py @@ -1,15 +1,23 @@ -from typing import Dict, Tuple +import os +import errno +from typing import Dict, Tuple, Optional -from common.xattr import getxattr as getattr1 -from common.xattr import setxattr as setattr1 +_cached_attributes: Dict[Tuple, Optional[bytes]] = {} -cached_attributes: Dict[Tuple, bytes] = {} -def getxattr(path: str, attr_name: str) -> bytes: - if (path, attr_name) not in cached_attributes: - response = getattr1(path, attr_name) - cached_attributes[(path, attr_name)] = response - return cached_attributes[(path, attr_name)] +def getxattr(path: str, attr_name: str) -> Optional[bytes]: + key = (path, attr_name) + if key not in _cached_attributes: + try: + response = os.getxattr(path, attr_name) + except OSError as e: + # ENODATA means attribute hasn't been set + if e.errno == errno.ENODATA: + response = None + else: + raise + _cached_attributes[key] = response + return _cached_attributes[key] def setxattr(path: str, attr_name: str, attr_value: bytes) -> None: - cached_attributes.pop((path, attr_name), None) - return setattr1(path, attr_name, attr_value) + _cached_attributes.pop((path, attr_name), None) + return os.setxattr(path, attr_name, attr_value)