Fix PersistentStorageDelegate APIs to be consistent about error repor…

…ting. Fixes project-chip#15671
bzbarsky-apple · Mar 1, 2022 · ee77cb7 · ee77cb7
1 parent 32f41f1
commit ee77cb7
Show file tree

Hide file tree

Showing 9 changed files with 91 additions and 24 deletions.
diff --git a/examples/chip-tool/config/PersistentStorage.cpp b/examples/chip-tool/config/PersistentStorage.cpp
@@ -104,7 +104,7 @@ CHIP_ERROR PersistentStorage::SyncGetKeyValue(const char * key, void * value, ui
 
     auto section = mConfig.sections[kDefaultSectionName];
     auto it      = section.find(key);
-    ReturnErrorCodeIf(it == section.end(), CHIP_ERROR_KEY_NOT_FOUND);
+    ReturnErrorCodeIf(it == section.end(), CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
 
     ReturnErrorCodeIf(!inipp::extract(section[key], iniValue), CHIP_ERROR_INVALID_ARGUMENT);
 
@@ -135,6 +135,9 @@ CHIP_ERROR PersistentStorage::SyncSetKeyValue(const char * key, const void * val
 CHIP_ERROR PersistentStorage::SyncDeleteKeyValue(const char * key)
 {
     auto section = mConfig.sections[kDefaultSectionName];
+    auto it      = section.find(key);
+    ReturnErrorCodeIf(it == section.end(), CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
+
     section.erase(key);
 
     mConfig.sections[kDefaultSectionName] = section;

diff --git a/examples/platform/linux/AppMain.cpp b/examples/platform/linux/AppMain.cpp
@@ -265,8 +265,16 @@ class MyServerStorageDelegate : public PersistentStorageDelegate
 {
     CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override
     {
-        ChipLogProgress(AppServer, "Retrieved value from server storage.");
-        return PersistedStorage::KeyValueStoreMgr().Get(key, buffer, size);
+        ChipLogProgress(AppServer, "Retrieving value from server storage.");
+        size_t bytesRead - 0;
+        CHIP_ERROR err = PersistedStorage::KeyValueStoreMgr().Get(key, buffer, size, &bytesRead);
+
+        if (err == CHIP_NO_ERROR)
+        {
+            ChipLogProgress(AppServer, "Retrieved value from server storage.");
+        }
+        size = static_cast<uint16_t>(bytesRead);
+        return err;
     }
 
     CHIP_ERROR SyncSetKeyValue(const char * key, const void * value, uint16_t size) override

diff --git a/src/app/server/Server.h b/src/app/server/Server.h
@@ -115,16 +115,15 @@ class Server
     {
         CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override
         {
-            size_t bytesRead;
-            ReturnErrorOnFailure(DeviceLayer::PersistedStorage::KeyValueStoreMgr().Get(key, buffer, size, &bytesRead));
-            if (!CanCastTo<uint16_t>(bytesRead))
+            size_t bytesRead = 0;
+            CHIP_ERROR err   = DeviceLayer::PersistedStorage::KeyValueStoreMgr().Get(key, buffer, size, &bytesRead);
+
+            if (err == CHIP_NO_ERROR)
             {
-                ChipLogDetail(AppServer, "0x%" PRIx32 " is too big to fit in uint16_t", static_cast<uint32_t>(bytesRead));
-                return CHIP_ERROR_BUFFER_TOO_SMALL;
+                ChipLogProgress(AppServer, "Retrieved from server storage: %s", key);
             }
-            ChipLogProgress(AppServer, "Retrieved from server storage: %s", key);
             size = static_cast<uint16_t>(bytesRead);
-            return CHIP_NO_ERROR;
+            return err;
         }
 
         CHIP_ERROR SyncSetKeyValue(const char * key, const void * value, uint16_t size) override

diff --git a/src/controller/python/ChipDeviceController-StorageDelegate.cpp b/src/controller/python/ChipDeviceController-StorageDelegate.cpp
@@ -34,7 +34,7 @@ CHIP_ERROR PythonPersistentStorageDelegate::SyncGetKeyValue(const char * key, vo
     auto val = mStorage.find(key);
     if (val == mStorage.end())
     {
-        return CHIP_ERROR_KEY_NOT_FOUND;
+        return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND;
     }
 
     if (value == nullptr)
@@ -46,14 +46,14 @@ CHIP_ERROR PythonPersistentStorageDelegate::SyncGetKeyValue(const char * key, vo
     if (size == 0)
     {
         size = neededSize;
-        return CHIP_ERROR_NO_MEMORY;
+        return CHIP_ERROR_BUFFER_TOO_SMALL;
     }
 
     if (size < neededSize)
     {
         memcpy(value, val->second.data(), size);
         size = neededSize;
-        return CHIP_ERROR_NO_MEMORY;
+        return CHIP_ERROR_BUFFER_TOO_SMALL;
     }
 
     memcpy(value, val->second.data(), neededSize);
@@ -71,6 +71,12 @@ CHIP_ERROR PythonPersistentStorageDelegate::SyncSetKeyValue(const char * key, co
 
 CHIP_ERROR PythonPersistentStorageDelegate::SyncDeleteKeyValue(const char * key)
 {
+    auto val = mStorage.find(key);
+    if (val == mStorage.end())
+    {
+        return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND;
+    }
+
     mStorage.erase(key);
     return CHIP_NO_ERROR;
 }
@@ -88,13 +94,13 @@ CHIP_ERROR StorageAdapter::SyncGetKeyValue(const char * key, void * value, uint1
     if (tmpSize == 0)
     {
         ChipLogDetail(Controller, "Key Not Found\n");
-        return CHIP_ERROR_KEY_NOT_FOUND;
+        return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND;
     }
     else if (size < tmpSize)
     {
         ChipLogDetail(Controller, "Buf not big enough\n");
         size = tmpSize;
-        return CHIP_ERROR_NO_MEMORY;
+        return CHIP_ERROR_BUFFER_TOO_SMALL;
     }
     else
     {
@@ -114,6 +120,14 @@ CHIP_ERROR StorageAdapter::SyncSetKeyValue(const char * key, const void * value,
 
 CHIP_ERROR StorageAdapter::SyncDeleteKeyValue(const char * key)
 {
+    uint8_t val[1];
+    uint16_t size  = 0;
+    CHIP_ERROR err = SyncGetKeyValue(key, val, size);
+    if (err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND)
+    {
+        return err;
+    }
+
     mDeleteKeyCb(mContext, key);
     return CHIP_NO_ERROR;
 }

diff --git a/src/controller/python/chip/internal/CommissionerImpl.cpp b/src/controller/python/chip/internal/CommissionerImpl.cpp
@@ -40,7 +40,10 @@ class ServerStorageDelegate : public chip::PersistentStorageDelegate
     CHIP_ERROR
     SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override
     {
-        return chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Get(key, buffer, size);
+        size_t bytesRead = 0;
+        CHIP_ERROR err   = chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Get(key, buffer, size, &bytesRead);
+        size             = static_cast<uint16_t>(bytesRead);
+        return err;
     }
 
     CHIP_ERROR SyncSetKeyValue(const char * key, const void * value, uint16_t size) override

diff --git a/src/darwin/Framework/CHIP/CHIPPersistentStorageDelegateBridge.mm b/src/darwin/Framework/CHIP/CHIPPersistentStorageDelegateBridge.mm
@@ -93,19 +93,20 @@
 
             if (decoded.length() > UINT16_MAX) {
                 error = CHIP_ERROR_BUFFER_TOO_SMALL;
+                size = 0;
             } else {
                 if (buffer != nullptr) {
                     memcpy(buffer, decoded.data(), std::min<size_t>(decoded.length(), size));
                     if (size < decoded.length()) {
-                        error = CHIP_ERROR_NO_MEMORY;
+                        error = CHIP_ERROR_BUFFER_TOO_SMALL;
                     }
                 } else {
                     error = CHIP_ERROR_NO_MEMORY;
                 }
                 size = static_cast<uint16_t>(decoded.length());
             }
         } else {
-            error = CHIP_ERROR_KEY_NOT_FOUND;
+            error = CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND;
         }
     });
     return error;

diff --git a/src/lib/core/CHIPPersistentStorageDelegate.h b/src/lib/core/CHIPPersistentStorageDelegate.h
@@ -45,6 +45,18 @@ class DLL_EXPORT PersistentStorageDelegate
      *                 The output length could be larger than input value. In
      *                 such cases, the user should allocate the buffer large
      *                 enough (>= output length), and call the API again.
+     *
+     * @return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND the key is unknown.
+     * @return CHIP_ERROR_BUFFER_TOO_SMALL the provided buffer is not big
+     *                                     enough.  In this case "size" will
+     *                                     indicate the needed buffer size.
+     *                                     Some data may or may not be placed
+     *                                     in "buffer" in this case; consumers
+     *                                     should not rely on that behavior.
+     *                                     CHIP_ERROR_BUFFER_TOO_SMALL combined
+     *                                     with setting "size" to 0 means the
+     *                                     actual size was too large to fit in
+     *                                     uint16_t.
      */
     virtual CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) = 0;
 
@@ -63,6 +75,8 @@ class DLL_EXPORT PersistentStorageDelegate
      *   Deletes the value for the key
      *
      * @param[in] key Key to be deleted
+     *
+     * @return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND the key is unknown.
      */
     virtual CHIP_ERROR SyncDeleteKeyValue(const char * key) = 0;
 };

diff --git a/src/lib/support/TestPersistentStorageDelegate.h b/src/lib/support/TestPersistentStorageDelegate.h
@@ -57,6 +57,8 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate, public F
 
     CHIP_ERROR SyncDeleteKeyValue(const char * key) override
     {
+        bool contains = mStorage.find(key) != mStorage.end();
+        VerifyOrReturnError(contains, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
         mStorage.erase(key);
         return CHIP_NO_ERROR;
     }

diff --git a/src/protocols/secure_channel/tests/TestCASESession.cpp b/src/protocols/secure_channel/tests/TestCASESession.cpp
@@ -294,17 +294,22 @@ class TestCASESessionPersistentStorageDelegate : public PersistentStorageDelegat
     {
         for (int i = 0; i < 16; i++)
         {
-            if (keys[i] != nullptr && keysize[i] != 0 && size >= valuesize[i])
+            if (keys[i] != nullptr && keysize[i] != 0 && strncmp(key, keys[i], keysize[i]) == 0)
             {
-                if (memcmp(key, keys[i], keysize[i]) == 0)
+                if (size >= valuesize[i])
                 {
                     memcpy(buffer, values[i], valuesize[i]);
                     size = valuesize[i];
                     return CHIP_NO_ERROR;
                 }
+                else
+                {
+                    size = (valuesize[i]);
+                    return CHIP_ERROR_BUFFER_TOO_SMALL;
+                }
             }
         }
-        return CHIP_ERROR_INTERNAL;
+        return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND;
     }
 
     CHIP_ERROR SyncSetKeyValue(const char * key, const void * value, uint16_t size) override
@@ -316,7 +321,7 @@ class TestCASESessionPersistentStorageDelegate : public PersistentStorageDelegat
                 keysize[i] = static_cast<uint16_t>(strlen(key));
                 keysize[i]++;
                 keys[i] = reinterpret_cast<char *>(chip::Platform::MemoryAlloc(keysize[i]));
-                strcpy(keys[i], key);
+                memcpy(keys[i], key, keysize[i]);
                 values[i] = reinterpret_cast<char *>(chip::Platform::MemoryAlloc(size));
                 memcpy(values[i], value, size);
                 valuesize[i] = size;
@@ -326,7 +331,25 @@ class TestCASESessionPersistentStorageDelegate : public PersistentStorageDelegat
         return CHIP_ERROR_INTERNAL;
     }
 
-    CHIP_ERROR SyncDeleteKeyValue(const char * key) override { return CHIP_NO_ERROR; }
+    CHIP_ERROR SyncDeleteKeyValue(const char * key) override
+    {
+        for (int i = 0; i < 16; i++)
+        {
+            if (keys[i] != nullptr && keysize[i] != 0 && strncmp(key, keys[i], keysize[i]) == 0)
+            {
+                Platform::MemoryFree(keys[i]);
+                keys[i] = nullptr;
+
+                if (values[i] != nullptr)
+                {
+                    Platform::MemoryFree(values[i]);
+                    values[i] = nullptr;
+                }
+                return CHIP_NO_ERROR;
+            }
+        }
+        return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND;
+    }
 
     CHIP_ERROR SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override
     {
@@ -341,7 +364,7 @@ class TestCASESessionPersistentStorageDelegate : public PersistentStorageDelegat
     CHIP_ERROR SyncDelete(FabricIndex fabricIndex, const char * key) override { return SyncDeleteKeyValue(key); };
 
 private:
-    char * keys[16];
+    char * keys[16]; // Not null-terminated
     void * values[16];
     uint16_t keysize[16];
     uint16_t valuesize[16];