From efb3bb19d7fc953e9bd1450ac4c98034fb36e48c Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 21 Dec 2020 12:02:02 -0800 Subject: [PATCH] Rationalize File timestamp callback Fixes #7775 Clean up the passing/setting of custom File time callbacks and add a host test verifying they work. Existing core was not passing custom timeCallbacks set at the FS level down to open()ed files, resulting in them calling the default time(nullptr) and reporting wrong file modify times. --- cores/esp8266/FS.cpp | 12 +++++----- cores/esp8266/FS.h | 7 +++--- cores/esp8266/FSImpl.h | 18 +++++++-------- libraries/LittleFS/src/LittleFS.cpp | 4 ++-- libraries/LittleFS/src/LittleFS.h | 4 ++-- tests/host/fs/test_fs.cpp | 31 -------------------------- tests/host/fs/test_fs.inc | 34 +++++++++++++++++++++++++++++ 7 files changed, 58 insertions(+), 52 deletions(-) diff --git a/cores/esp8266/FS.cpp b/cores/esp8266/FS.cpp index d73be017a3..f897a52d1d 100644 --- a/cores/esp8266/FS.cpp +++ b/cores/esp8266/FS.cpp @@ -206,6 +206,7 @@ void File::setTimeCallback(time_t (*cb)(void)) { if (!_p) return; _p->setTimeCallback(cb); + _timeCallback = cb; } File Dir::openFile(const char* mode) { @@ -221,7 +222,7 @@ File Dir::openFile(const char* mode) { } File f(_impl->openFile(om, am), _baseFS); - f.setTimeCallback(timeCallback); + f.setTimeCallback(_timeCallback); return f; } @@ -287,7 +288,7 @@ void Dir::setTimeCallback(time_t (*cb)(void)) { if (!_impl) return; _impl->setTimeCallback(cb); - timeCallback = cb; + _timeCallback = cb; } @@ -304,7 +305,7 @@ bool FS::begin() { DEBUGV("#error: FS: no implementation"); return false; } - _impl->setTimeCallback(timeCallback); + _impl->setTimeCallback(_timeCallback); bool ret = _impl->begin(); DEBUGV("%s\n", ret? "": "#error: FS could not start"); return ret; @@ -367,7 +368,7 @@ File FS::open(const char* path, const char* mode) { return File(); } File f(_impl->open(path, om, am), this); - f.setTimeCallback(timeCallback); + f.setTimeCallback(_timeCallback); return f; } @@ -388,7 +389,7 @@ Dir FS::openDir(const char* path) { } DirImplPtr p = _impl->openDir(path); Dir d(p, this); - d.setTimeCallback(timeCallback); + d.setTimeCallback(_timeCallback); return d; } @@ -444,6 +445,7 @@ void FS::setTimeCallback(time_t (*cb)(void)) { if (!_impl) return; _impl->setTimeCallback(cb); + _timeCallback = cb; } diff --git a/cores/esp8266/FS.h b/cores/esp8266/FS.h index b1f1c528aa..99c71cdcea 100644 --- a/cores/esp8266/FS.h +++ b/cores/esp8266/FS.h @@ -118,6 +118,7 @@ class File : public Stream protected: FileImplPtr _p; + time_t (*_timeCallback)(void) = nullptr; // Arduino SD class emulation std::shared_ptr _fakeDir; @@ -145,7 +146,7 @@ class Dir { protected: DirImplPtr _impl; FS *_baseFS; - time_t (*timeCallback)(void) = nullptr; + time_t (*_timeCallback)(void) = nullptr; }; // Backwards compatible, <4GB filesystem usage @@ -198,7 +199,7 @@ class SPIFFSConfig : public FSConfig class FS { public: - FS(FSImplPtr impl) : _impl(impl) { timeCallback = _defaultTimeCB; } + FS(FSImplPtr impl) : _impl(impl) { _timeCallback = _defaultTimeCB; } bool setConfig(const FSConfig &cfg); @@ -240,7 +241,7 @@ class FS protected: FSImplPtr _impl; FSImplPtr getImpl() { return _impl; } - time_t (*timeCallback)(void); + time_t (*_timeCallback)(void) = nullptr; static time_t _defaultTimeCB(void) { return time(NULL); } }; diff --git a/cores/esp8266/FSImpl.h b/cores/esp8266/FSImpl.h index 373b26f2ca..801a8d9c0d 100644 --- a/cores/esp8266/FSImpl.h +++ b/cores/esp8266/FSImpl.h @@ -45,8 +45,8 @@ class FileImpl { // Filesystems *may* support a timestamp per-file, so allow the user to override with // their own callback for *this specific* file (as opposed to the FSImpl call of the - // same name. The default implementation simply returns time(&null) - virtual void setTimeCallback(time_t (*cb)(void)) { timeCallback = cb; } + // same name. The default implementation simply returns time(null) + virtual void setTimeCallback(time_t (*cb)(void)) { _timeCallback = cb; } // Return the last written time for a file. Undefined when called on a writable file // as the FS is allowed to return either the time of the last write() operation or the @@ -56,7 +56,7 @@ class FileImpl { virtual time_t getCreationTime() { return 0; } // Default is to not support timestamps protected: - time_t (*timeCallback)(void) = nullptr; + time_t (*_timeCallback)(void) = nullptr; }; enum OpenMode { @@ -90,11 +90,11 @@ class DirImpl { // Filesystems *may* support a timestamp per-file, so allow the user to override with // their own callback for *this specific* file (as opposed to the FSImpl call of the - // same name. The default implementation simply returns time(&null) - virtual void setTimeCallback(time_t (*cb)(void)) { timeCallback = cb; } + // same name. The default implementation simply returns time(null) + virtual void setTimeCallback(time_t (*cb)(void)) { _timeCallback = cb; } protected: - time_t (*timeCallback)(void) = nullptr; + time_t (*_timeCallback)(void) = nullptr; }; class FSImpl { @@ -118,11 +118,11 @@ class FSImpl { // Filesystems *may* support a timestamp per-file, so allow the user to override with // their own callback for all files on this FS. The default implementation simply - // returns the present time as reported by time(&null) - virtual void setTimeCallback(time_t (*cb)(void)) { timeCallback = cb; } + // returns the present time as reported by time(null) + virtual void setTimeCallback(time_t (*cb)(void)) { _timeCallback = cb; } protected: - time_t (*timeCallback)(void) = nullptr; + time_t (*_timeCallback)(void) = nullptr; }; } // namespace fs diff --git a/libraries/LittleFS/src/LittleFS.cpp b/libraries/LittleFS/src/LittleFS.cpp index 7fd4351fcb..16f4df55e9 100644 --- a/libraries/LittleFS/src/LittleFS.cpp +++ b/libraries/LittleFS/src/LittleFS.cpp @@ -70,14 +70,14 @@ FileImplPtr LittleFSImpl::open(const char* path, OpenMode openMode, AccessMode a } time_t creation = 0; - if (timeCallback && (openMode & OM_CREATE)) { + if (_timeCallback && (openMode & OM_CREATE)) { // O_CREATE means we *may* make the file, but not if it already exists. // See if it exists, and only if not update the creation time int rc = lfs_file_open(&_lfs, fd.get(), path, LFS_O_RDONLY); if (rc == 0) { lfs_file_close(&_lfs, fd.get()); // It exists, don't update create time } else { - creation = timeCallback(); // File didn't exist or otherwise, so we're going to create this time + creation = _timeCallback(); // File didn't exist or otherwise, so we're going to create this time } } diff --git a/libraries/LittleFS/src/LittleFS.h b/libraries/LittleFS/src/LittleFS.h index f78680f662..495b63421e 100644 --- a/libraries/LittleFS/src/LittleFS.h +++ b/libraries/LittleFS/src/LittleFS.h @@ -424,7 +424,7 @@ class LittleFSFileImpl : public FileImpl lfs_file_close(_fs->getFS(), _getFD()); _opened = false; DEBUGV("lfs_file_close: fd=%p\n", _getFD()); - if (timeCallback && (_flags & LFS_O_WRONLY)) { + if (_timeCallback && (_flags & LFS_O_WRONLY)) { // If the file opened with O_CREAT, write the creation time attribute if (_creation) { int rc = lfs_setattr(_fs->getFS(), _name.get(), 'c', (const void *)&_creation, sizeof(_creation)); @@ -433,7 +433,7 @@ class LittleFSFileImpl : public FileImpl } } // Add metadata with last write time - time_t now = timeCallback(); + time_t now = _timeCallback(); int rc = lfs_setattr(_fs->getFS(), _name.get(), 't', (const void *)&now, sizeof(now)); if (rc < 0) { DEBUGV("Unable to set last write time on '%s' to %d\n", _name.get(), now); diff --git a/tests/host/fs/test_fs.cpp b/tests/host/fs/test_fs.cpp index e82d212fb0..fb73a6a271 100644 --- a/tests/host/fs/test_fs.cpp +++ b/tests/host/fs/test_fs.cpp @@ -162,35 +162,4 @@ TEST_CASE("SD.h FILE_WRITE macro is append", "[fs]") REQUIRE(u == 0); } -// SDFS timestamp setter (#7682) -static time_t _my_time(void) -{ - struct tm t; - bzero(&t, sizeof(t)); - t.tm_year = 120; - t.tm_mon = 9; - t.tm_mday = 22; - t.tm_hour = 12; - t.tm_min = 13; - t.tm_sec = 14; - return mktime(&t); -} - -TEST_CASE("SDFS timeCallback") -{ - SDFS_MOCK_DECLARE(64, 8, 512, ""); - REQUIRE(SDFS.begin()); - REQUIRE(SD.begin(4)); - - SDFS.setTimeCallback(_my_time); - File f = SD.open("/file.txt", "w"); - f.write("Had we but world enough, and time,"); - f.close(); - time_t expected = _my_time(); - f = SD.open("/file.txt", "r"); - REQUIRE(abs(f.getCreationTime() - expected) < 60); // FAT has less precision in timestamp than time_t - REQUIRE(abs(f.getLastWrite() - expected) < 60); // FAT has less precision in timestamp than time_t - f.close(); -} - }; diff --git a/tests/host/fs/test_fs.inc b/tests/host/fs/test_fs.inc index 683f3a99ba..b7e7692a46 100644 --- a/tests/host/fs/test_fs.inc +++ b/tests/host/fs/test_fs.inc @@ -220,6 +220,40 @@ TEST_CASE(TESTPRE "Rewriting file frees space immediately (#7426)", TESTPAT) } } +#if FSTYPE != SPIFFS + +// Timestamp setter (#7682, #7775) +static time_t _my_time(void) +{ + struct tm t; + bzero(&t, sizeof(t)); + t.tm_year = 120; + t.tm_mon = 9; + t.tm_mday = 22; + t.tm_hour = 12; + t.tm_min = 13; + t.tm_sec = 14; + return mktime(&t); +} + +TEST_CASE("Verify timeCallback works properly") +{ + FS_MOCK_DECLARE(64, 8, 512, ""); + REQUIRE(FSTYPE.begin()); + + FSTYPE.setTimeCallback(_my_time); + File f = FSTYPE.open("/file.txt", "w"); + f.write("Had we but world enough, and time,"); + f.close(); + time_t expected = _my_time(); + f = FSTYPE.open("/file.txt", "r"); + REQUIRE(abs(f.getCreationTime() - expected) < 60); // FAT has less precision in timestamp than time_t + REQUIRE(abs(f.getLastWrite() - expected) < 60); // FAT has less precision in timestamp than time_t + f.close(); +} + +#endif + #ifdef FS_HAS_DIRS #if FSTYPE != SDFS