From a3b9a54b8f862d5374c0ac4545686840a473e6d3 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 1 Mar 2022 10:56:05 -0500 Subject: [PATCH] Fix PersistentStorageDelegate APIs to be consistent about error reporting. Fixes https://github.com/project-chip/connectedhomeip/issues/15671 --- .../chip-tool/config/PersistentStorage.cpp | 5 ++- examples/platform/linux/AppMain.cpp | 12 +++++-- src/app/server/Server.h | 13 ++++--- .../ChipDeviceController-StorageDelegate.cpp | 24 ++++++++++--- .../python/chip/internal/CommissionerImpl.cpp | 5 ++- .../CHIPPersistentStorageDelegateBridge.mm | 5 +-- src/lib/core/CHIPPersistentStorageDelegate.h | 14 ++++++++ .../support/TestPersistentStorageDelegate.h | 2 ++ .../secure_channel/tests/TestCASESession.cpp | 35 +++++++++++++++---- 9 files changed, 91 insertions(+), 24 deletions(-) diff --git a/examples/chip-tool/config/PersistentStorage.cpp b/examples/chip-tool/config/PersistentStorage.cpp index 20444da6d59b02..bbae5f5e76655f 100644 --- 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 index 147a2c7684e60e..c56550d6835337 100644 --- 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(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 index bf19fc4e84ed3a..c03af254406d3e 100644 --- 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(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(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(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 index 93706bc16c6a58..2e255e13e074b4 100644 --- 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 index d45fd7597ce582..846c97b7298bbe 100644 --- 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(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 index 94ad61c5ebfc6d..5b0b68973b0dcd 100644 --- a/src/darwin/Framework/CHIP/CHIPPersistentStorageDelegateBridge.mm +++ b/src/darwin/Framework/CHIP/CHIPPersistentStorageDelegateBridge.mm @@ -93,11 +93,12 @@ if (decoded.length() > UINT16_MAX) { error = CHIP_ERROR_BUFFER_TOO_SMALL; + size = 0; } else { if (buffer != nullptr) { memcpy(buffer, decoded.data(), std::min(decoded.length(), size)); if (size < decoded.length()) { - error = CHIP_ERROR_NO_MEMORY; + error = CHIP_ERROR_BUFFER_TOO_SMALL; } } else { error = CHIP_ERROR_NO_MEMORY; @@ -105,7 +106,7 @@ size = static_cast(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 index eca9a40fd75c27..f8640acb7d1c5b 100644 --- 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 index bd05826f9e9048..899501b3dbd0e5 100644 --- 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 index 06d13c05fd875a..5599a82ceb25d7 100644 --- 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(strlen(key)); keysize[i]++; keys[i] = reinterpret_cast(chip::Platform::MemoryAlloc(keysize[i])); - strcpy(keys[i], key); + memcpy(keys[i], key, keysize[i]); values[i] = reinterpret_cast(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];