From 15710e4e41e81129d1052fc90c709f14fdda2d8c Mon Sep 17 00:00:00 2001 From: Ben Allfree Date: Wed, 19 Aug 2026 22:17:56 -0700 Subject: [PATCH] fix: atomic saves for prefs, ACL, regions and blobs Co-authored-by: Cursor --- examples/companion_radio/DataStore.cpp | 174 ++++++++++++++----------- src/helpers/ClientACL.cpp | 59 ++++----- src/helpers/CommonCLI.cpp | 15 +-- src/helpers/ConfigSerializer.cpp | 43 ++++++ src/helpers/ConfigSerializer.h | 10 ++ src/helpers/RegionMap.cpp | 67 +++++----- src/helpers/RegionMap.h | 2 + 7 files changed, 212 insertions(+), 158 deletions(-) diff --git a/examples/companion_radio/DataStore.cpp b/examples/companion_radio/DataStore.cpp index 06c56a7a44..8c2edf7623 100644 --- a/examples/companion_radio/DataStore.cpp +++ b/examples/companion_radio/DataStore.cpp @@ -1,5 +1,6 @@ #include #include "DataStore.h" +#include #if defined(EXTRAFS) || defined(QSPIFLASH) #define MAX_BLOBRECS 100 @@ -31,7 +32,8 @@ DataStore::DataStore(FILESYSTEM& fs, FILESYSTEM& fsExtra, mesh::RTCClock& clock) } #endif -static File openWrite(FILESYSTEM* fs, const char* filename) { +// One-time migration into an empty destination FS only. +static File migrateOpenWrite(FILESYSTEM* fs, const char* filename) { #if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) fs->remove(filename); return fs->open(filename, FILE_O_WRITE); @@ -247,13 +249,7 @@ void DataStore::loadPrefsInt(const char *filename, NodePrefs& _prefs) { } bool DataStore::savePrefs(NodePrefs& _prefs) { - File file = openWrite(_fs, "/prefs.json"); - if (file) { - bool success = _prefs.saveSerial(file); - file.close(); - return success; - } - return false; + return saveConfigJsonAtomic(_fs, _prefs, "/prefs.json", "/.prefs.json.new"); } void DataStore::loadContacts(DataStoreHost* host) { @@ -287,37 +283,43 @@ File file = openRead(_getContactsChannelsFS(), "/contacts3"); } } -void DataStore::saveContacts(DataStoreHost* host, bool (*filter)(const ContactInfo& c)) { - File file = openWrite(_getContactsChannelsFS(), "/contacts3"); - if (file) { - uint32_t idx = 0; - ContactInfo c; - uint8_t unused = 0; - - while (host->getContactForSave(idx, c)) { - if (filter && !filter(c)) { - idx++; // advance to next contact - continue; - } - bool success = (file.write(c.id.pub_key, 32) == 32); - success = success && (file.write((uint8_t *)&c.name, 32) == 32); - success = success && (file.write(&c.type, 1) == 1); - success = success && (file.write(&c.flags, 1) == 1); - success = success && (file.write(&unused, 1) == 1); - success = success && (file.write((uint8_t *)&c.sync_since, 4) == 4); - success = success && (file.write((uint8_t *)&c.out_path_len, 1) == 1); - success = success && (file.write((uint8_t *)&c.last_advert_timestamp, 4) == 4); - success = success && (file.write(c.out_path, 64) == 64); - success = success && (file.write((uint8_t *)&c.lastmod, 4) == 4); - success = success && (file.write((uint8_t *)&c.gps_lat, 4) == 4); - success = success && (file.write((uint8_t *)&c.gps_lon, 4) == 4); - - if (!success) break; // write failed - - idx++; // advance to next contact +struct SaveContactsCtx { + DataStoreHost* host; + bool (*filter)(const ContactInfo& c); +}; + +static bool writeContactsBody(File& file, void* ctx) { + SaveContactsCtx* c = (SaveContactsCtx*) ctx; + uint32_t idx = 0; + ContactInfo contact; + uint8_t unused = 0; + + while (c->host->getContactForSave(idx, contact)) { + if (c->filter && !c->filter(contact)) { + idx++; + continue; } - file.close(); + bool success = (file.write(contact.id.pub_key, 32) == 32); + success = success && (file.write((uint8_t*) &contact.name, 32) == 32); + success = success && (file.write(&contact.type, 1) == 1); + success = success && (file.write(&contact.flags, 1) == 1); + success = success && (file.write(&unused, 1) == 1); + success = success && (file.write((uint8_t*) &contact.sync_since, 4) == 4); + success = success && (file.write((uint8_t*) &contact.out_path_len, 1) == 1); + success = success && (file.write((uint8_t*) &contact.last_advert_timestamp, 4) == 4); + success = success && (file.write(contact.out_path, 64) == 64); + success = success && (file.write((uint8_t*) &contact.lastmod, 4) == 4); + success = success && (file.write((uint8_t*) &contact.gps_lat, 4) == 4); + success = success && (file.write((uint8_t*) &contact.gps_lon, 4) == 4); + if (!success) return false; + idx++; } + return true; +} + +void DataStore::saveContacts(DataStoreHost* host, bool (*filter)(const ContactInfo& c)) { + SaveContactsCtx ctx = {host, filter}; + writeFileAtomic(_getContactsChannelsFS(), "/contacts3", "/.contacts3.new", writeContactsBody, &ctx); } void DataStore::loadChannels(DataStoreHost* host) { @@ -345,24 +347,30 @@ void DataStore::loadChannels(DataStoreHost* host) { } } -void DataStore::saveChannels(DataStoreHost* host) { - File file = openWrite(_getContactsChannelsFS(), "/channels2"); - if (file) { - uint8_t channel_idx = 0; - ChannelDetails ch; - uint8_t unused[4]; - memset(unused, 0, 4); - - while (host->getChannelForSave(channel_idx, ch)) { - bool success = (file.write(unused, 4) == 4); - success = success && (file.write((uint8_t *)ch.name, 32) == 32); - success = success && (file.write((uint8_t *)ch.channel.secret, 32) == 32); - - if (!success) break; // write failed - channel_idx++; - } - file.close(); +struct SaveChannelsCtx { + DataStoreHost* host; +}; + +static bool writeChannelsBody(File& file, void* ctx) { + SaveChannelsCtx* c = (SaveChannelsCtx*) ctx; + uint8_t channel_idx = 0; + ChannelDetails ch; + uint8_t unused[4]; + memset(unused, 0, 4); + + while (c->host->getChannelForSave(channel_idx, ch)) { + bool success = (file.write(unused, 4) == 4); + success = success && (file.write((uint8_t*) ch.name, 32) == 32); + success = success && (file.write((uint8_t*) ch.channel.secret, 32) == 32); + if (!success) return false; + channel_idx++; } + return true; +} + +void DataStore::saveChannels(DataStoreHost* host) { + SaveChannelsCtx ctx = {host}; + writeFileAtomic(_getContactsChannelsFS(), "/channels2", "/.channels2.new", writeChannelsBody, &ctx); } #if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) @@ -376,17 +384,24 @@ struct BlobRec { uint8_t data[MAX_ADVERT_PKT_LEN]; }; +struct InitAdvBlobsCtx { + int max_recs; +}; + +static bool writeAdvBlobsInitBody(File& file, void* ctx) { + InitAdvBlobsCtx* c = (InitAdvBlobsCtx*) ctx; + BlobRec zeroes; + memset(&zeroes, 0, sizeof(zeroes)); + for (int i = 0; i < c->max_recs; i++) { + if (file.write((uint8_t*) &zeroes, sizeof(zeroes)) != sizeof(zeroes)) return false; + } + return true; +} + void DataStore::checkAdvBlobFile() { if (!_getContactsChannelsFS()->exists("/adv_blobs")) { - File file = openWrite(_getContactsChannelsFS(), "/adv_blobs"); - if (file) { - BlobRec zeroes; - memset(&zeroes, 0, sizeof(zeroes)); - for (int i = 0; i < MAX_BLOBRECS; i++) { // pre-allocate to fixed size - file.write((uint8_t *) &zeroes, sizeof(zeroes)); - } - file.close(); - } + InitAdvBlobsCtx ctx = {MAX_BLOBRECS}; + writeFileAtomic(_getContactsChannelsFS(), "/adv_blobs", "/.adv_blobs.new", writeAdvBlobsInitBody, &ctx); } } @@ -395,7 +410,7 @@ void DataStore::migrateToSecondaryFS() { if (!_fsExtra->exists("/adv_blobs")) { if (_fs->exists("/adv_blobs")) { File oldAdvBlobs = openRead(_fs, "/adv_blobs"); - File newAdvBlobs = openWrite(_fsExtra, "/adv_blobs"); + File newAdvBlobs = migrateOpenWrite(_fsExtra, "/adv_blobs"); if (oldAdvBlobs && newAdvBlobs) { BlobRec rec; @@ -416,7 +431,7 @@ void DataStore::migrateToSecondaryFS() { if (!_fsExtra->exists("/contacts3")) { if (_fs->exists("/contacts3")) { File oldFile = openRead(_fs, "/contacts3"); - File newFile = openWrite(_fsExtra, "/contacts3"); + File newFile = migrateOpenWrite(_fsExtra, "/contacts3"); if (oldFile && newFile) { uint8_t buf[64]; @@ -433,7 +448,7 @@ void DataStore::migrateToSecondaryFS() { if (!_fsExtra->exists("/channels2")) { if (_fs->exists("/channels2")) { File oldFile = openRead(_fs, "/channels2"); - File newFile = openWrite(_fsExtra, "/channels2"); + File newFile = migrateOpenWrite(_fsExtra, "/channels2"); if (oldFile && newFile) { uint8_t buf[64]; @@ -451,7 +466,7 @@ void DataStore::migrateToSecondaryFS() { if (_fsExtra->exists("/_main.id")) { if (_fs->exists("/_main.id")) {_fs->remove("/_main.id");} File oldFile = openRead(_fsExtra, "/_main.id"); - File newFile = openWrite(_fs, "/_main.id"); + File newFile = migrateOpenWrite(_fs, "/_main.id"); if (oldFile && newFile) { uint8_t buf[64]; @@ -467,7 +482,7 @@ void DataStore::migrateToSecondaryFS() { if (_fsExtra->exists("/new_prefs")) { if (_fs->exists("/new_prefs")) {_fs->remove("/new_prefs");} File oldFile = openRead(_fsExtra, "/new_prefs"); - File newFile = openWrite(_fs, "/new_prefs"); + File newFile = migrateOpenWrite(_fs, "/new_prefs"); if (oldFile && newFile) { uint8_t buf[64]; @@ -578,19 +593,24 @@ uint8_t DataStore::getBlobByKey(const uint8_t key[], int key_len, uint8_t dest_b return 0; // not found } +struct BlobWriteCtx { + const uint8_t* buf; + uint8_t len; +}; + +static bool writeBlobBody(File& file, void* ctx) { + BlobWriteCtx* c = (BlobWriteCtx*) ctx; + return file.write(c->buf, c->len) == c->len; +} + bool DataStore::putBlobByKey(const uint8_t key[], int key_len, const uint8_t src_buf[], uint8_t len) { char path[64]; makeBlobPath(key, key_len, path, sizeof(path)); - File f = openWrite(_fs, path); - if (f) { - int n = f.write(src_buf, len); - f.close(); - if (n == len) return true; // success! - - _fs->remove(path); // blob was only partially written! - } - return false; // error + char tmp_path[72]; + snprintf(tmp_path, sizeof(tmp_path), "%s.new", path); + BlobWriteCtx ctx = {src_buf, len}; + return writeFileAtomic(_fs, path, tmp_path, writeBlobBody, &ctx); } bool DataStore::deleteBlobByKey(const uint8_t key[], int key_len) { diff --git a/src/helpers/ClientACL.cpp b/src/helpers/ClientACL.cpp index 1282382737..c6a9b22e6b 100644 --- a/src/helpers/ClientACL.cpp +++ b/src/helpers/ClientACL.cpp @@ -1,14 +1,30 @@ #include "ClientACL.h" - -static File openWrite(FILESYSTEM* _fs, const char* filename) { - #if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) - _fs->remove(filename); - return _fs->open(filename, FILE_O_WRITE); - #elif defined(RP2040_PLATFORM) - return _fs->open(filename, "w"); - #else - return _fs->open(filename, "w", true); - #endif +#include "ConfigSerializer.h" + +struct SaveAclCtx { + ClientACL* acl; + bool (*filter)(ClientInfo*); +}; + +static bool writeAclBody(File& file, void* ctx) { + SaveAclCtx* c = (SaveAclCtx*) ctx; + uint8_t unused[2]; + memset(unused, 0, sizeof(unused)); + + for (int i = 0; i < c->acl->getNumClients(); i++) { + auto client = c->acl->getClientByIdx(i); + if (client->permissions == 0 || (c->filter && !c->filter(client))) continue; + + bool success = (file.write(client->id.pub_key, 32) == 32); + success = success && (file.write((uint8_t*) &client->permissions, 1) == 1); + success = success && (file.write((uint8_t*) &client->extra.room.sync_since, 4) == 4); + success = success && (file.write(unused, 2) == 2); + success = success && (file.write((uint8_t*) &client->out_path_len, 1) == 1); + success = success && (file.write(client->out_path, 64) == 64); + success = success && (file.write(client->shared_secret, PUB_KEY_SIZE) == PUB_KEY_SIZE); + if (!success) return false; + } + return true; } void ClientACL::load(FILESYSTEM* fs, const mesh::LocalIdentity& self_id) { @@ -54,27 +70,8 @@ void ClientACL::load(FILESYSTEM* fs, const mesh::LocalIdentity& self_id) { void ClientACL::save(FILESYSTEM* fs, bool (*filter)(ClientInfo*)) { _fs = fs; - File file = openWrite(_fs, "/s_contacts"); - if (file) { - uint8_t unused[2]; - memset(unused, 0, sizeof(unused)); - - for (int i = 0; i < num_clients; i++) { - auto c = &clients[i]; - if (c->permissions == 0 || (filter && !filter(c))) continue; // skip deleted entries, or by filter function - - bool success = (file.write(c->id.pub_key, 32) == 32); - success = success && (file.write((uint8_t *) &c->permissions, 1) == 1); - success = success && (file.write((uint8_t *) &c->extra.room.sync_since, 4) == 4); - success = success && (file.write(unused, 2) == 2); - success = success && (file.write((uint8_t *)&c->out_path_len, 1) == 1); - success = success && (file.write(c->out_path, 64) == 64); - success = success && (file.write(c->shared_secret, PUB_KEY_SIZE) == PUB_KEY_SIZE); - - if (!success) break; // write failed - } - file.close(); - } + SaveAclCtx ctx = {this, filter}; + writeFileAtomic(_fs, "/s_contacts", "/.s_contacts.new", writeAclBody, &ctx); } bool ClientACL::clear() { diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index b318bb58e8..99e83e3579 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -141,20 +141,7 @@ void CommonCLI::loadPrefsInt(FILESYSTEM* fs, const char* filename) { // Legacy } bool CommonCLI::savePrefs(FILESYSTEM* fs) { -#if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) - fs->remove("/prefs.json"); - File file = fs->open("/prefs.json", FILE_O_WRITE); -#elif defined(RP2040_PLATFORM) - File file = fs->open("/prefs.json", "w"); -#else - File file = fs->open("/prefs.json", "w", true); -#endif - if (file) { - bool success = _prefs->saveSerial(file); - file.close(); - return success; - } - return false; + return saveConfigJsonAtomic(fs, *_prefs, "/prefs.json", "/.prefs.json.new"); } #define MIN_LOCAL_ADVERT_INTERVAL 60 diff --git a/src/helpers/ConfigSerializer.cpp b/src/helpers/ConfigSerializer.cpp index adff147f47..1e221d3ff1 100644 --- a/src/helpers/ConfigSerializer.cpp +++ b/src/helpers/ConfigSerializer.cpp @@ -1,5 +1,48 @@ #include "ConfigSerializer.h" +static File openNewFile(FILESYSTEM* fs, const char* path) { +#if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) + return fs->open(path, FILE_O_WRITE); +#elif defined(RP2040_PLATFORM) + return fs->open(path, "w"); +#else + return fs->open(path, "w", true); +#endif +} + +bool writeFileAtomic(FILESYSTEM* fs, const char* final_path, const char* tmp_path, FileWriteFn writer, void* ctx) { + if (!fs || !final_path || !tmp_path || !writer) return false; + + fs->remove(tmp_path); + File file = openNewFile(fs, tmp_path); + if (!file) return false; + + bool success = writer(file, ctx); + file.close(); + if (!success) { + fs->remove(tmp_path); + return false; + } + if (!fs->rename(tmp_path, final_path)) { + fs->remove(tmp_path); + return false; + } + return true; +} + +struct SaveSerialCtx { + ConfigSerializer* obj; +}; + +static bool saveSerialWriter(File& file, void* ctx) { + return ((SaveSerialCtx*) ctx)->obj->saveSerial(file); +} + +bool saveConfigJsonAtomic(FILESYSTEM* fs, ConfigSerializer& obj, const char* final_path, const char* tmp_path) { + SaveSerialCtx ctx = {&obj}; + return writeFileAtomic(fs, final_path, tmp_path, saveSerialWriter, &ctx); +} + bool ConfigSerializer::saveSerial(Stream& s) { Context context(&s, OP::WRITE); _context = &context; // set the context for structure() call diff --git a/src/helpers/ConfigSerializer.h b/src/helpers/ConfigSerializer.h index 7e6d6f2a69..9aded8873c 100644 --- a/src/helpers/ConfigSerializer.h +++ b/src/helpers/ConfigSerializer.h @@ -66,3 +66,13 @@ class ConfigSerializer { bool loadSerial(Stream& s); bool saveSerial(Stream& s); }; + +#include "IdentityStore.h" + +typedef bool (*FileWriteFn)(File& file, void* ctx); + +// Write to tmp_path via writer, then rename over final_path. Keeps the old file on failed writes. +bool writeFileAtomic(FILESYSTEM* fs, const char* final_path, const char* tmp_path, FileWriteFn writer, void* ctx); + +// Write JSON to tmp_path, then rename over final_path. Keeps the old file on failed writes. +bool saveConfigJsonAtomic(FILESYSTEM* fs, ConfigSerializer& obj, const char* final_path, const char* tmp_path); diff --git a/src/helpers/RegionMap.cpp b/src/helpers/RegionMap.cpp index 4667e0038e..13508035bd 100644 --- a/src/helpers/RegionMap.cpp +++ b/src/helpers/RegionMap.cpp @@ -1,5 +1,7 @@ #include "RegionMap.h" #include +#include +#include #include // helper class for region map exporter, we emulate Stream with a safe buffer writer. @@ -58,15 +60,31 @@ static const char* skip_hash(const char* name) { return *name == '#' ? name + 1 : name; } -static File openWrite(FILESYSTEM* _fs, const char* filename) { - #if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) - _fs->remove(filename); - return _fs->open(filename, FILE_O_WRITE); - #elif defined(RP2040_PLATFORM) - return _fs->open(filename, "w"); - #else - return _fs->open(filename, "w", true); - #endif +bool RegionMap::saveBodyWriter(File& file, void* ctx) { + return ((RegionMap*) ctx)->writeSaveBody(file); +} + +bool RegionMap::writeSaveBody(File& file) const { + uint8_t pad[128]; + memset(pad, 0, sizeof(pad)); + + bool success = file.write(pad, 3) == 3; + success = success && file.write((uint8_t*) &default_id, sizeof(default_id)) == sizeof(default_id); + success = success && file.write((uint8_t*) &home_id, sizeof(home_id)) == sizeof(home_id); + success = success && file.write((uint8_t*) &wildcard.flags, sizeof(wildcard.flags)) == sizeof(wildcard.flags); + success = success && file.write((uint8_t*) &next_id, sizeof(next_id)) == sizeof(next_id); + if (!success) return false; + + for (int i = 0; i < num_regions; i++) { + auto r = ®ions[i]; + success = file.write((uint8_t*) &r->id, sizeof(r->id)) == sizeof(r->id); + success = success && file.write((uint8_t*) &r->parent, sizeof(r->parent)) == sizeof(r->parent); + success = success && file.write((uint8_t*) r->name, sizeof(r->name)) == sizeof(r->name); + success = success && file.write((uint8_t*) &r->flags, sizeof(r->flags)) == sizeof(r->flags); + success = success && file.write(pad, sizeof(pad)) == sizeof(pad); + if (!success) return false; + } + return true; } bool RegionMap::load(FILESYSTEM* _fs, const char* path) { @@ -117,33 +135,10 @@ bool RegionMap::load(FILESYSTEM* _fs, const char* path) { } bool RegionMap::save(FILESYSTEM* _fs, const char* path) { - File file = openWrite(_fs, path ? path : "/regions2"); - if (file) { - uint8_t pad[128]; - memset(pad, 0, sizeof(pad)); - - bool success = file.write(pad, 3) == 3; // reserved header - success = success && file.write((uint8_t *) &default_id, sizeof(default_id)) == sizeof(default_id); - success = success && file.write((uint8_t *) &home_id, sizeof(home_id)) == sizeof(home_id); - success = success && file.write((uint8_t *) &wildcard.flags, sizeof(wildcard.flags)) == sizeof(wildcard.flags); - success = success && file.write((uint8_t *) &next_id, sizeof(next_id)) == sizeof(next_id); - - if (success) { - for (int i = 0; i < num_regions; i++) { - auto r = ®ions[i]; - - success = file.write((uint8_t *) &r->id, sizeof(r->id)) == sizeof(r->id); - success = success && file.write((uint8_t *) &r->parent, sizeof(r->parent)) == sizeof(r->parent); - success = success && file.write((uint8_t *) r->name, sizeof(r->name)) == sizeof(r->name); - success = success && file.write((uint8_t *) &r->flags, sizeof(r->flags)) == sizeof(r->flags); - success = success && file.write(pad, sizeof(pad)) == sizeof(pad); - if (!success) break; // write failed - } - } - file.close(); - return success; - } - return false; // failed + const char* final_path = path ? path : "/regions2"; + char tmp_path[32]; + snprintf(tmp_path, sizeof(tmp_path), "/.%s.new", final_path + 1); + return writeFileAtomic(_fs, final_path, tmp_path, saveBodyWriter, this); } RegionEntry* RegionMap::putRegion(const char* name, uint16_t parent_id, uint16_t id) { diff --git a/src/helpers/RegionMap.h b/src/helpers/RegionMap.h index 5eb1442983..11208dc8ce 100644 --- a/src/helpers/RegionMap.h +++ b/src/helpers/RegionMap.h @@ -27,6 +27,8 @@ class RegionMap { RegionEntry regions[MAX_REGION_ENTRIES]; RegionEntry wildcard; + bool writeSaveBody(File& file) const; + static bool saveBodyWriter(File& file, void* ctx); void printChildRegions(int indent, const RegionEntry* parent, Stream& out) const; public: