Fix max_keys reset limitation

Persist the max_keys value through a soft-reset, also prohibit max_keys set under predefined default value (16)

Author: theamirocohen

features/storage/nvstore/TESTS/nvstore/functionality/main.cpp

@@ -449,12 +449,13 @@ static void thread_test_worker()
 static void nvstore_multi_thread_test()
 {
 #ifdef MBED_CONF_RTOS_PRESENT
-    int i;
+    int i, result;
     uint16_t size;
     uint16_t key;
     int ret;
     char *dummy;
-    uint16_t max_possible_keys;
+    uint16_t max_possible_keys, actual_len_bytes = 0;
+    char test[] = "Test", read_buf[10] = {};
 
     NVStore &nvstore = NVStore::get_instance();
 
@@ -483,6 +484,10 @@ static void nvstore_multi_thread_test()
     TEST_SKIP_UNLESS_MESSAGE(max_possible_keys >= max_possible_keys_threshold,
                              "Max possible keys below threshold. Test skipped.");
 
+    nvstore.set_max_keys(max_test_keys + 1);
+    result = nvstore.set(max_test_keys, strlen(test), test);
+    TEST_ASSERT_EQUAL(NVSTORE_SUCCESS, result);
+
     thr_test_data->stop_threads = false;
     for (key = 0; key < thr_test_data->max_keys; key++) {
         for (i = 0; i < thr_test_num_buffs; i++) {
@@ -527,6 +532,12 @@ static void nvstore_multi_thread_test()
     for (key = 0; key < thr_test_data->max_keys; key++) {
         thread_test_check_key(key);
     }
+
+    result = nvstore.get(max_test_keys, sizeof(read_buf), read_buf, actual_len_bytes);
+    TEST_ASSERT_EQUAL(NVSTORE_SUCCESS, result);
+    TEST_ASSERT_EQUAL(strlen(test), actual_len_bytes);
+    TEST_ASSERT_EQUAL_UINT8_ARRAY(test, read_buf, strlen(test));
+
     goto clean;
 
 mem_fail:

features/storage/nvstore/source/nvstore.cpp

@@ -61,8 +61,8 @@ static const unsigned int owner_bit_pos = 12;
 
 typedef struct {
     uint16_t version;
-    uint16_t reserved1;
-    uint32_t reserved2;
+    uint16_t max_keys;
+    uint32_t reserved;
 } master_record_data_t;
 
 static const uint32_t min_area_size = 4096;
@@ -171,11 +171,54 @@ uint16_t NVStore::get_max_possible_keys()
 
 void NVStore::set_max_keys(uint16_t num_keys)
 {
+    uint16_t key = 0, old_max_keys = 0;
+
     MBED_ASSERT(num_keys < get_max_possible_keys());
+
+    if (num_keys < NVSTORE_NUM_PREDEFINED_KEYS) {
+        return;
+    }
+
+    if (!_init_done) {
+        int ret = init();
+        if (ret != NVSTORE_SUCCESS) {
+            return;
+        }
+    }
+
+    _mutex->lock();
+
+    //check if there are values that might be discarded
+    if (num_keys < _max_keys) {
+        for (key = num_keys; key < _max_keys; key++) {
+            if (_offset_by_key[key] != 0) {
+                return;
+            }
+        }
+    }
+
+    old_max_keys = _max_keys;
     _max_keys = num_keys;
-    // User is allowed to change number of keys. As this affects init, need to deinitialize now.
-    // Don't call init right away - it is lazily called by get/set functions if needed.
-    deinit();
+
+    // Invoke GC to write new max_keys to master record
+    garbage_collection(no_key, 0, 0, 0, NULL, std::min(_max_keys, old_max_keys));
+
+    // Reallocate _offset_by_key with new size
+    if (_max_keys != old_max_keys) {
+        // Reallocate _offset_by_key with new size
+        uint32_t *old_offset_by_key = (uint32_t *) _offset_by_key;
+        uint32_t *new_offset_by_key = new uint32_t[_max_keys];
+        MBED_ASSERT(new_offset_by_key);
+
+        // Copy old content to new table
+        memset(new_offset_by_key, 0, sizeof(uint32_t) * _max_keys);
+        memcpy(new_offset_by_key, old_offset_by_key, sizeof(uint32_t) * std::min(_max_keys, old_max_keys));
+
+        _offset_by_key = new_offset_by_key;
+        delete[] old_offset_by_key;
+    }
+
+    _mutex->unlock();
 }
 
 int NVStore::flash_read_area(uint8_t area, uint32_t offset, uint32_t size, void *buf)
@@ -444,8 +487,8 @@ int NVStore::write_master_record(uint8_t area, uint16_t version, uint32_t &next_
     master_record_data_t master_rec;
 
     master_rec.version = version;
-    master_rec.reserved1 = 0;
-    master_rec.reserved2 = 0;
+    master_rec.max_keys = _max_keys;
+    master_rec.reserved = 0;
     return write_record(area, 0, master_record_key, 0, 0, sizeof(master_rec),
                         &master_rec, next_offset);
 }
@@ -518,7 +561,7 @@ int NVStore::copy_record(uint8_t from_area, uint32_t from_offset, uint32_t to_of
     return NVSTORE_SUCCESS;
 }
 
-int NVStore::garbage_collection(uint16_t key, uint16_t flags, uint8_t owner, uint16_t buf_size, const void *buf)
+int NVStore::garbage_collection(uint16_t key, uint16_t flags, uint8_t owner, uint16_t buf_size, const void *buf, uint16_t num_keys)
 {
     uint32_t curr_offset, new_area_offset, next_offset, curr_owner;
     int ret;
@@ -542,7 +585,7 @@ int NVStore::garbage_collection(uint16_t key, uint16_t flags, uint8_t owner, uin
 
     // Now iterate on all types, and copy the ones who have valid offsets (meaning that they exist)
     // to the other area.
-    for (key = 0; key < _max_keys; key++) {
+    for (key = 0; key < num_keys; key++) {
         curr_offset = _offset_by_key[key];
         uint16_t save_flags = curr_offset & offs_by_key_flag_mask & ~offs_by_key_area_mask;
         curr_area = (uint8_t)(curr_offset >> offs_by_key_area_bit_pos) & 1;
@@ -579,7 +622,6 @@ int NVStore::garbage_collection(uint16_t key, uint16_t flags, uint8_t owner, uin
     return ret;
 }
 
-
 int NVStore::do_get(uint16_t key, uint16_t buf_size, void *buf, uint16_t &actual_size,
                     int validate_only)
 {
@@ -684,7 +726,7 @@ int NVStore::do_set(uint16_t key, uint16_t buf_size, const void *buf, uint16_t f
 
     // If we cross the area limit, we need to invoke GC.
     if (new_free_space >= _size) {
-        ret = garbage_collection(key, flags, owner, buf_size, buf);
+        ret = garbage_collection(key, flags, owner, buf_size, buf, _max_keys);
         _mutex->unlock();
         return ret;
     }
@@ -800,6 +842,7 @@ int NVStore::init()
     uint16_t key;
     uint16_t flags;
     uint16_t versions[NVSTORE_NUM_AREAS];
+    uint16_t keys[NVSTORE_NUM_AREAS];
     uint16_t actual_size;
     uint8_t owner;
 
@@ -818,13 +861,6 @@ int NVStore::init()
         return NVSTORE_SUCCESS;
     }
 
-    _offset_by_key = new uint32_t[_max_keys];
-    MBED_ASSERT(_offset_by_key);
-
-    for (key = 0; key < _max_keys; key++) {
-        _offset_by_key[key] = 0;
-    }
-
     _mutex = new PlatformMutex;
     MBED_ASSERT(_mutex);
 
@@ -841,10 +877,12 @@ int NVStore::init()
 
     calc_validate_area_params();
 
+    //retrieve max keys from master record
     for (uint8_t area = 0; area < NVSTORE_NUM_AREAS; area++) {
         area_state[area] = NVSTORE_AREA_STATE_NONE;
         free_space_offset_of_area[area] =  0;
         versions[area] = 0;
+        keys[area] = 0;
 
         _size = std::min(_size, _flash_area_params[area].size);
 
@@ -878,6 +916,7 @@ int NVStore::init()
             continue;
         }
         versions[area] = master_rec.version;
+        keys[area] = master_rec.max_keys;
 
         // Place _free_space_offset after the master record (for the traversal,
         // which takes place after this loop).
@@ -888,6 +927,17 @@ int NVStore::init()
         // that we found our active area.
         _active_area = area;
         _active_area_version = versions[area];
+        if (!keys[area]) {
+            keys[area] = NVSTORE_NUM_PREDEFINED_KEYS;
+        }
+        _max_keys = keys[area];
+    }
+
+    _offset_by_key = new uint32_t[_max_keys];
+    MBED_ASSERT(_offset_by_key);
+
+    for (key = 0; key < _max_keys; key++) {
+        _offset_by_key[key] = 0;
     }
 
     // In case we have two empty areas, arbitrarily assign 0 to the active one.
@@ -920,9 +970,9 @@ int NVStore::init()
         MBED_ASSERT(ret == NVSTORE_SUCCESS);
 
         // In case we have a faulty record, this probably means that the system crashed when written.
-        // Perform a garbage collection, to make the the other area valid.
+        // Perform a garbage collection, to make the other area valid.
         if (!valid) {
-            ret = garbage_collection(no_key, 0, 0, 0, NULL);
+            ret = garbage_collection(no_key, 0, 0, 0, NULL, _max_keys);
             break;
         }
         if (flags & delete_item_flag) {

features/storage/nvstore/source/nvstore.h

@@ -272,7 +272,6 @@ class NVStore : private mbed::NonCopyable<NVStore> {
      */
     int get_area_params(uint8_t area, uint32_t &address, size_t &size);
 
-
 private:
     typedef struct {
         uint32_t address;
@@ -417,10 +416,11 @@ class NVStore : private mbed::NonCopyable<NVStore> {
      * @param[in]  owner                  Owner.
      * @param[in]  buf_size               Data size (bytes).
      * @param[in]  buf                    Data buffer.
+     * @param[in]  num_keys               number of keys.
      *
      * @returns 0 for success, nonzero for failure.
      */
-    int garbage_collection(uint16_t key, uint16_t flags, uint8_t owner, uint16_t buf_size, const void *buf);
+    int garbage_collection(uint16_t key, uint16_t flags, uint8_t owner, uint16_t buf_size, const void *buf, uint16_t num_keys);
 
     /**
      * @brief Actual logics of get API (covers also get size API).