Skip to content

Commit 35d22ed

Browse files
Rationalize File timestamp callback (#7785)
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.
1 parent f1dc6e9 commit 35d22ed

File tree

7 files changed

+58
-52
lines changed

7 files changed

+58
-52
lines changed

cores/esp8266/FS.cpp

+7-5
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,7 @@ void File::setTimeCallback(time_t (*cb)(void)) {
206206
if (!_p)
207207
return;
208208
_p->setTimeCallback(cb);
209+
_timeCallback = cb;
209210
}
210211

211212
File Dir::openFile(const char* mode) {
@@ -221,7 +222,7 @@ File Dir::openFile(const char* mode) {
221222
}
222223

223224
File f(_impl->openFile(om, am), _baseFS);
224-
f.setTimeCallback(timeCallback);
225+
f.setTimeCallback(_timeCallback);
225226
return f;
226227
}
227228

@@ -287,7 +288,7 @@ void Dir::setTimeCallback(time_t (*cb)(void)) {
287288
if (!_impl)
288289
return;
289290
_impl->setTimeCallback(cb);
290-
timeCallback = cb;
291+
_timeCallback = cb;
291292
}
292293

293294

@@ -304,7 +305,7 @@ bool FS::begin() {
304305
DEBUGV("#error: FS: no implementation");
305306
return false;
306307
}
307-
_impl->setTimeCallback(timeCallback);
308+
_impl->setTimeCallback(_timeCallback);
308309
bool ret = _impl->begin();
309310
DEBUGV("%s\n", ret? "": "#error: FS could not start");
310311
return ret;
@@ -367,7 +368,7 @@ File FS::open(const char* path, const char* mode) {
367368
return File();
368369
}
369370
File f(_impl->open(path, om, am), this);
370-
f.setTimeCallback(timeCallback);
371+
f.setTimeCallback(_timeCallback);
371372
return f;
372373
}
373374

@@ -388,7 +389,7 @@ Dir FS::openDir(const char* path) {
388389
}
389390
DirImplPtr p = _impl->openDir(path);
390391
Dir d(p, this);
391-
d.setTimeCallback(timeCallback);
392+
d.setTimeCallback(_timeCallback);
392393
return d;
393394
}
394395

@@ -444,6 +445,7 @@ void FS::setTimeCallback(time_t (*cb)(void)) {
444445
if (!_impl)
445446
return;
446447
_impl->setTimeCallback(cb);
448+
_timeCallback = cb;
447449
}
448450

449451

cores/esp8266/FS.h

+4-3
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@ class File : public Stream
118118

119119
protected:
120120
FileImplPtr _p;
121+
time_t (*_timeCallback)(void) = nullptr;
121122

122123
// Arduino SD class emulation
123124
std::shared_ptr<Dir> _fakeDir;
@@ -145,7 +146,7 @@ class Dir {
145146
protected:
146147
DirImplPtr _impl;
147148
FS *_baseFS;
148-
time_t (*timeCallback)(void) = nullptr;
149+
time_t (*_timeCallback)(void) = nullptr;
149150
};
150151

151152
// Backwards compatible, <4GB filesystem usage
@@ -198,7 +199,7 @@ class SPIFFSConfig : public FSConfig
198199
class FS
199200
{
200201
public:
201-
FS(FSImplPtr impl) : _impl(impl) { timeCallback = _defaultTimeCB; }
202+
FS(FSImplPtr impl) : _impl(impl) { _timeCallback = _defaultTimeCB; }
202203

203204
bool setConfig(const FSConfig &cfg);
204205

@@ -240,7 +241,7 @@ class FS
240241
protected:
241242
FSImplPtr _impl;
242243
FSImplPtr getImpl() { return _impl; }
243-
time_t (*timeCallback)(void);
244+
time_t (*_timeCallback)(void) = nullptr;
244245
static time_t _defaultTimeCB(void) { return time(NULL); }
245246
};
246247

cores/esp8266/FSImpl.h

+9-9
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,8 @@ class FileImpl {
4545

4646
// Filesystems *may* support a timestamp per-file, so allow the user to override with
4747
// their own callback for *this specific* file (as opposed to the FSImpl call of the
48-
// same name. The default implementation simply returns time(&null)
49-
virtual void setTimeCallback(time_t (*cb)(void)) { timeCallback = cb; }
48+
// same name. The default implementation simply returns time(null)
49+
virtual void setTimeCallback(time_t (*cb)(void)) { _timeCallback = cb; }
5050

5151
// Return the last written time for a file. Undefined when called on a writable file
5252
// as the FS is allowed to return either the time of the last write() operation or the
@@ -56,7 +56,7 @@ class FileImpl {
5656
virtual time_t getCreationTime() { return 0; } // Default is to not support timestamps
5757

5858
protected:
59-
time_t (*timeCallback)(void) = nullptr;
59+
time_t (*_timeCallback)(void) = nullptr;
6060
};
6161

6262
enum OpenMode {
@@ -90,11 +90,11 @@ class DirImpl {
9090

9191
// Filesystems *may* support a timestamp per-file, so allow the user to override with
9292
// their own callback for *this specific* file (as opposed to the FSImpl call of the
93-
// same name. The default implementation simply returns time(&null)
94-
virtual void setTimeCallback(time_t (*cb)(void)) { timeCallback = cb; }
93+
// same name. The default implementation simply returns time(null)
94+
virtual void setTimeCallback(time_t (*cb)(void)) { _timeCallback = cb; }
9595

9696
protected:
97-
time_t (*timeCallback)(void) = nullptr;
97+
time_t (*_timeCallback)(void) = nullptr;
9898
};
9999

100100
class FSImpl {
@@ -118,11 +118,11 @@ class FSImpl {
118118

119119
// Filesystems *may* support a timestamp per-file, so allow the user to override with
120120
// their own callback for all files on this FS. The default implementation simply
121-
// returns the present time as reported by time(&null)
122-
virtual void setTimeCallback(time_t (*cb)(void)) { timeCallback = cb; }
121+
// returns the present time as reported by time(null)
122+
virtual void setTimeCallback(time_t (*cb)(void)) { _timeCallback = cb; }
123123

124124
protected:
125-
time_t (*timeCallback)(void) = nullptr;
125+
time_t (*_timeCallback)(void) = nullptr;
126126
};
127127

128128
} // namespace fs

libraries/LittleFS/src/LittleFS.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -70,14 +70,14 @@ FileImplPtr LittleFSImpl::open(const char* path, OpenMode openMode, AccessMode a
7070
}
7171

7272
time_t creation = 0;
73-
if (timeCallback && (openMode & OM_CREATE)) {
73+
if (_timeCallback && (openMode & OM_CREATE)) {
7474
// O_CREATE means we *may* make the file, but not if it already exists.
7575
// See if it exists, and only if not update the creation time
7676
int rc = lfs_file_open(&_lfs, fd.get(), path, LFS_O_RDONLY);
7777
if (rc == 0) {
7878
lfs_file_close(&_lfs, fd.get()); // It exists, don't update create time
7979
} else {
80-
creation = timeCallback(); // File didn't exist or otherwise, so we're going to create this time
80+
creation = _timeCallback(); // File didn't exist or otherwise, so we're going to create this time
8181
}
8282
}
8383

libraries/LittleFS/src/LittleFS.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -424,7 +424,7 @@ class LittleFSFileImpl : public FileImpl
424424
lfs_file_close(_fs->getFS(), _getFD());
425425
_opened = false;
426426
DEBUGV("lfs_file_close: fd=%p\n", _getFD());
427-
if (timeCallback && (_flags & LFS_O_WRONLY)) {
427+
if (_timeCallback && (_flags & LFS_O_WRONLY)) {
428428
// If the file opened with O_CREAT, write the creation time attribute
429429
if (_creation) {
430430
int rc = lfs_setattr(_fs->getFS(), _name.get(), 'c', (const void *)&_creation, sizeof(_creation));
@@ -433,7 +433,7 @@ class LittleFSFileImpl : public FileImpl
433433
}
434434
}
435435
// Add metadata with last write time
436-
time_t now = timeCallback();
436+
time_t now = _timeCallback();
437437
int rc = lfs_setattr(_fs->getFS(), _name.get(), 't', (const void *)&now, sizeof(now));
438438
if (rc < 0) {
439439
DEBUGV("Unable to set last write time on '%s' to %d\n", _name.get(), now);

tests/host/fs/test_fs.cpp

-31
Original file line numberDiff line numberDiff line change
@@ -162,35 +162,4 @@ TEST_CASE("SD.h FILE_WRITE macro is append", "[fs]")
162162
REQUIRE(u == 0);
163163
}
164164

165-
// SDFS timestamp setter (#7682)
166-
static time_t _my_time(void)
167-
{
168-
struct tm t;
169-
bzero(&t, sizeof(t));
170-
t.tm_year = 120;
171-
t.tm_mon = 9;
172-
t.tm_mday = 22;
173-
t.tm_hour = 12;
174-
t.tm_min = 13;
175-
t.tm_sec = 14;
176-
return mktime(&t);
177-
}
178-
179-
TEST_CASE("SDFS timeCallback")
180-
{
181-
SDFS_MOCK_DECLARE(64, 8, 512, "");
182-
REQUIRE(SDFS.begin());
183-
REQUIRE(SD.begin(4));
184-
185-
SDFS.setTimeCallback(_my_time);
186-
File f = SD.open("/file.txt", "w");
187-
f.write("Had we but world enough, and time,");
188-
f.close();
189-
time_t expected = _my_time();
190-
f = SD.open("/file.txt", "r");
191-
REQUIRE(abs(f.getCreationTime() - expected) < 60); // FAT has less precision in timestamp than time_t
192-
REQUIRE(abs(f.getLastWrite() - expected) < 60); // FAT has less precision in timestamp than time_t
193-
f.close();
194-
}
195-
196165
};

tests/host/fs/test_fs.inc

+34
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,40 @@ TEST_CASE(TESTPRE "Rewriting file frees space immediately (#7426)", TESTPAT)
220220
}
221221
}
222222

223+
#if FSTYPE != SPIFFS
224+
225+
// Timestamp setter (#7682, #7775)
226+
static time_t _my_time(void)
227+
{
228+
struct tm t;
229+
bzero(&t, sizeof(t));
230+
t.tm_year = 120;
231+
t.tm_mon = 9;
232+
t.tm_mday = 22;
233+
t.tm_hour = 12;
234+
t.tm_min = 13;
235+
t.tm_sec = 14;
236+
return mktime(&t);
237+
}
238+
239+
TEST_CASE("Verify timeCallback works properly")
240+
{
241+
FS_MOCK_DECLARE(64, 8, 512, "");
242+
REQUIRE(FSTYPE.begin());
243+
244+
FSTYPE.setTimeCallback(_my_time);
245+
File f = FSTYPE.open("/file.txt", "w");
246+
f.write("Had we but world enough, and time,");
247+
f.close();
248+
time_t expected = _my_time();
249+
f = FSTYPE.open("/file.txt", "r");
250+
REQUIRE(abs(f.getCreationTime() - expected) < 60); // FAT has less precision in timestamp than time_t
251+
REQUIRE(abs(f.getLastWrite() - expected) < 60); // FAT has less precision in timestamp than time_t
252+
f.close();
253+
}
254+
255+
#endif
256+
223257
#ifdef FS_HAS_DIRS
224258

225259
#if FSTYPE != SDFS

0 commit comments

Comments
 (0)