bug meshtastic#4184 WIP do not merge

src/SafeFile.cpp

@@ -0,0 +1,92 @@
+#include "SafeFile.h"
+
+#ifdef FSCom
+
+SafeFile::SafeFile(const char *_filename, bool fullAtomic) : filename(_filename), f(FSCom), fullAtomic(fullAtomic)
+{
+    if (!fullAtomic)
+        FSCom.remove(filename.c_str()); // Nuke the old file to make space (ignore if it !exists)
+
+    String filenameTmp = filename;
+    filenameTmp += ".tmp";
+    f.open(filenameTmp.c_str(), FILE_O_WRITE);
+}
+
+size_t SafeFile::write(uint8_t ch)
+{
+    if (!f.isOpen())
+        return 0;
+
+    hash ^= ch;
+    return f.write(ch);
+}
+
+size_t SafeFile::write(const uint8_t *buffer, size_t size)
+{
+    if (!f.isOpen())
+        return 0;
+
+    for (size_t i = 0; i < size; i++) {
+        hash ^= buffer[i];
+    }
+    return f.write((uint8_t const *)buffer, size); // This nasty cast is _IMPORTANT_ otherwise the correct adafruit method does
+                                                   // not get used (they made a mistake in their typing)
+}
+
+/**
+ * Atomically close the file (deleting any old versions) and readback the contents to confirm the hash matches
+ *
+ * @return false for failure
+ */
+bool SafeFile::close()
+{
+    if (!f.isOpen())
+        return false;
+
+    f.close();
+    if (!testReadback())
+        return false;
+
+    // brief window of risk here ;-)
+    if (fullAtomic && FSCom.exists(filename.c_str()) && !FSCom.remove(filename.c_str())) {
+        LOG_ERROR("Can't remove old pref file\n");
+        return false;
+    }
+
+    String filenameTmp = filename;
+    filenameTmp += ".tmp";
+    if (!renameFile(filenameTmp.c_str(), filename.c_str())) {
+        LOG_ERROR("Error: can't rename new pref file\n");
+        return false;
+    }
+
+    return true;
+}
+
+/// Read our (closed) tempfile back in and compare the hash
+bool SafeFile::testReadback()
+{
+    String filenameTmp = filename;
+    filenameTmp += ".tmp";
+    auto f2 = FSCom.open(filenameTmp.c_str(), FILE_O_READ);
+    if (!f2.isOpen()) {
+        LOG_ERROR("Can't open tmp file for readback\n");
+        return false;
+    }
+
+    int c = 0;
+    uint8_t test_hash = 0;
+    while ((c = f2.read()) >= 0) {
+        test_hash ^= (uint8_t)c;
+    }
+    f2.close();
+
+    if (test_hash != hash) {
+        LOG_ERROR("Readback failed hash mismatch\n");
+        return false;
+    }
+
+    return true;
+}
+
+#endif

src/SafeFile.h

@@ -0,0 +1,45 @@
+#pragma once
+
+#include "FSCommon.h"
+#include "configuration.h"
+
+/**
+ * This class provides 'safe'/paranoid file writing.
+ *
+ * Some of our filesystems (in particular the nrf52) may have bugs beneath our layer.  Therefore we want to
+ * be very careful about how we write files.  This class provides a restricted (Stream only) writing API for writing to files.
+ *
+ * Notably:
+ * - we keep a simple xor hash of all characters that were written.
+ * - We do not allow seeking (because we want to maintain our hash)
+ * - we provide an close() method which is similar to close but returns false if we were unable to successfully write the
+ * file.  Also this method
+ * - atomically replaces any old version of the file on the disk with our new file (after first rereading the file from the disk
+ * to confirm the hash matches)
+ * - Some files are super huge so we can't do the full atomic rename/copy (because of filesystem size limits).  If !fullAtomic
+ * then we still do the readback to verify file is valid so higher level code can handle failures.
+ */
+class SafeFile : public Print
+{
+  public:
+    SafeFile(char const *filepath, bool fullAtomic = false);
+
+    virtual size_t write(uint8_t);
+    virtual size_t write(const uint8_t *buffer, size_t size);
+
+    /**
+     * Atomically close the file (deleting any old versions) and readback the contents to confirm the hash matches
+     *
+     * @return false for failure
+     */
+    bool close();
+
+  private:
+    /// Read our (closed) tempfile back in and compare the hash
+    bool testReadback();
+
+    String filename;
+    File f;
+    bool fullAtomic;
+    uint8_t hash = 0;
+};

src/mesh/NodeDB.cpp

@@ -14,6 +14,7 @@
 #include "PowerFSM.h"
 #include "RTC.h"
 #include "Router.h"
+#include "SafeFile.h"
 #include "TypeConversions.h"
 #include "error.h"
 #include "main.h"
@@ -54,6 +55,27 @@ meshtastic_LocalModuleConfig moduleConfig;
 meshtastic_ChannelFile channelFile;
 meshtastic_OEMStore oemStore;
 
+// These are not publically exposed - copied from InternalFileSystem.cpp
+// #define FLASH_NRF52_PAGE_SIZE   4096
+// #define LFS_FLASH_TOTAL_SIZE  (7*FLASH_NRF52_PAGE_SIZE)
+// #define LFS_BLOCK_SIZE        128
+
+/// List all files in the FS and test write and readback.
+/// Useful for filesystem stress testing - normally stripped from build by the linker.
+void flashTest()
+{
+    auto filesManifest = getFiles("/", 5);
+
+    uint32_t totalSize = 0;
+    for (size_t i = 0; i < filesManifest.size(); i++) {
+        LOG_INFO("File %s (size %d)\n", filesManifest[i].file_name, filesManifest[i].size_bytes);
+        totalSize += filesManifest[i].size_bytes;
+    }
+    LOG_INFO("%d files (total size %u)\n", filesManifest.size(), totalSize);
+    // LOG_INFO("Filesystem block size %u, total bytes %u", LFS_FLASH_TOTAL_SIZE, LFS_BLOCK_SIZE);
+    nodeDB->saveToDisk();
+}
+
 bool meshtastic_DeviceState_callback(pb_istream_t *istream, pb_ostream_t *ostream, const pb_field_iter_t *field)
 {
     if (ostream) {
@@ -697,32 +719,24 @@ bool NodeDB::saveProto(const char *filename, size_t protoSize, const pb_msgdesc_
 {
     bool okay = false;
 #ifdef FSCom
-    // static DeviceState scratch; We no longer read into a tempbuf because this structure is 15KB of valuable RAM
-    String filenameTmp = filename;
-    filenameTmp += ".tmp";
-    auto f = FSCom.open(filenameTmp.c_str(), FILE_O_WRITE);
-    if (f) {
-        LOG_INFO("Saving %s\n", filename);
-        pb_ostream_t stream = {&writecb, &f, protoSize};
+    auto f = SafeFile(filename);
 
-        if (!pb_encode(&stream, fields, dest_struct)) {
-            LOG_ERROR("Error: can't encode protobuf %s\n", PB_GET_ERROR(&stream));
-        } else {
-            okay = true;
-        }
-        f.flush();
-        f.close();
+    LOG_INFO("Saving %s\n", filename);
+    pb_ostream_t stream = {&writecb, static_cast<Print *>(&f), protoSize};
 
-        // brief window of risk here ;-)
-        if (FSCom.exists(filename) && !FSCom.remove(filename)) {
-            LOG_WARN("Can't remove old pref file\n");
-        }
-        if (!renameFile(filenameTmp.c_str(), filename)) {
-            LOG_ERROR("Error: can't rename new pref file\n");
-        }
+    if (!pb_encode(&stream, fields, dest_struct)) {
+        LOG_ERROR("Error: can't encode protobuf %s\n", PB_GET_ERROR(&stream));
     } else {
+        okay = true;
+    }
+
+    bool writeSucceeded = f.close();
+
+    if (!okay || !writeSucceeded) {
         LOG_ERROR("Can't write prefs\n");
-#ifdef ARCH_NRF52
+        // kevinh FIXME - I'm removing all this because if we know that the filesystem is toast at this point the only thing we
+        // _can_ do is to reformat and fill with our valid info from RAM.  To do otherwise risks losing valuable user config data.
+#if 0 // def ARCH_NRF52
         static uint8_t failedCounter = 0;
         failedCounter++;
         if (failedCounter >= 2) {
@@ -742,30 +756,33 @@ bool NodeDB::saveProto(const char *filename, size_t protoSize, const pb_msgdesc_
     return okay;
 }
 
-void NodeDB::saveChannelsToDisk()
+bool NodeDB::saveChannelsToDisk()
 {
 #ifdef FSCom
     FSCom.mkdir("/prefs");
 #endif
-    saveProto(channelFileName, meshtastic_ChannelFile_size, &meshtastic_ChannelFile_msg, &channelFile);
+    return saveProto(channelFileName, meshtastic_ChannelFile_size, &meshtastic_ChannelFile_msg, &channelFile);
 }
 
-void NodeDB::saveDeviceStateToDisk()
+bool NodeDB::saveDeviceStateToDisk()
 {
 #ifdef FSCom
     FSCom.mkdir("/prefs");
 #endif
-    saveProto(prefFileName, sizeof(devicestate) + numMeshNodes * meshtastic_NodeInfoLite_size, &meshtastic_DeviceState_msg,
-              &devicestate);
+    // Note: if MAX_NUM_NODES=100 and meshtastic_NodeInfoLite_size=166,
+    return saveProto(prefFileName, sizeof(devicestate) + numMeshNodes * meshtastic_NodeInfoLite_size, &meshtastic_DeviceState_msg,
+                     &devicestate);
 }
 
-void NodeDB::saveToDisk(int saveWhat)
+bool NodeDB::saveToDisk(int saveWhat)
 {
+    bool success = true;
+
 #ifdef FSCom
     FSCom.mkdir("/prefs");
 #endif
     if (saveWhat & SEGMENT_DEVICESTATE) {
-        saveDeviceStateToDisk();
+        success &= saveDeviceStateToDisk();
     }
 
     if (saveWhat & SEGMENT_CONFIG) {
@@ -777,7 +794,7 @@ void NodeDB::saveToDisk(int saveWhat)
         config.has_network = true;
         config.has_bluetooth = true;
 
-        saveProto(configFileName, meshtastic_LocalConfig_size, &meshtastic_LocalConfig_msg, &config);
+        success &= saveProto(configFileName, meshtastic_LocalConfig_size, &meshtastic_LocalConfig_msg, &config);
     }
 
     if (saveWhat & SEGMENT_MODULECONFIG) {
@@ -794,12 +811,15 @@ void NodeDB::saveToDisk(int saveWhat)
         moduleConfig.has_audio = true;
         moduleConfig.has_paxcounter = true;
 
-        saveProto(moduleConfigFileName, meshtastic_LocalModuleConfig_size, &meshtastic_LocalModuleConfig_msg, &moduleConfig);
+        success &=
+            saveProto(moduleConfigFileName, meshtastic_LocalModuleConfig_size, &meshtastic_LocalModuleConfig_msg, &moduleConfig);
     }
 
     if (saveWhat & SEGMENT_CHANNELS) {
-        saveChannelsToDisk();
+        success &= saveChannelsToDisk();
     }
+
+    return success;
 }
 
 const meshtastic_NodeInfoLite *NodeDB::readNextMeshNode(uint32_t &readIndex)
@@ -1059,4 +1079,4 @@ void recordCriticalError(meshtastic_CriticalErrorCode code, uint32_t address, co
     LOG_ERROR("A critical failure occurred, portduino is exiting...");
     exit(2);
 #endif
-}
+}

src/mesh/NodeDB.h

@@ -72,7 +72,8 @@ class NodeDB
     NodeDB();
 
     /// write to flash
-    void saveToDisk(int saveWhat = SEGMENT_CONFIG | SEGMENT_MODULECONFIG | SEGMENT_DEVICESTATE | SEGMENT_CHANNELS),
+    /// @return true if the save was successful
+    bool saveToDisk(int saveWhat = SEGMENT_CONFIG | SEGMENT_MODULECONFIG | SEGMENT_DEVICESTATE | SEGMENT_CHANNELS),
         saveChannelsToDisk(), saveDeviceStateToDisk();
 
     /** Reinit radio config if needed, because either:
@@ -228,4 +229,4 @@ extern uint32_t error_address;
      ModuleConfig_RangeTestConfig_size + ModuleConfig_SerialConfig_size + ModuleConfig_StoreForwardConfig_size +                 \
      ModuleConfig_TelemetryConfig_size + ModuleConfig_size)
 
-// Please do not remove this comment, it makes trunk and compiler happy at the same time.
+// Please do not remove this comment, it makes trunk and compiler happy at the same time.

src/mesh/mesh-pb-constants.cpp

@@ -58,7 +58,7 @@ bool readcb(pb_istream_t *stream, uint8_t *buf, size_t count)
 /// Write to an arduino file
 bool writecb(pb_ostream_t *stream, const uint8_t *buf, size_t count)
 {
-    File *file = (File *)stream->state;
+    auto file = (Print *)stream->state;
     // LOG_DEBUG("writing %d bytes to protobuf file\n", count);
     return file->write(buf, count) == count;
 }
@@ -71,4 +71,4 @@ bool is_in_helper(uint32_t n, const uint32_t *array, pb_size_t count)
             return true;
 
     return false;
-}
+}