Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PersistentStorageDelegate APIs to be consistent about error reporting #15675

Merged
merged 1 commit into from
Mar 2, 2022
Merged

Fix PersistentStorageDelegate APIs to be consistent about error reporting #15675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion examples/chip-tool/config/PersistentStorage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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;
Expand Down
12 changes: 10 additions & 2 deletions examples/platform/linux/AppMain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 6 additions & 7 deletions src/app/server/Server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 19 additions & 5 deletions src/controller/python/ChipDeviceController-StorageDelegate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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);
Expand All @@ -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;
}
Expand All @@ -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
{
Expand All @@ -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;
}
Expand Down
5 changes: 4 additions & 1 deletion src/controller/python/chip/internal/CommissionerImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions src/darwin/Framework/CHIP/CHIPPersistentStorageDelegateBridge.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
14 changes: 14 additions & 0 deletions src/lib/core/CHIPPersistentStorageDelegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
};
Expand Down
2 changes: 2 additions & 0 deletions src/lib/support/TestPersistentStorageDelegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
35 changes: 29 additions & 6 deletions src/protocols/secure_channel/tests/TestCASESession.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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]);
jepenven-silabs marked this conversation as resolved.
Show resolved Hide resolved
values[i] = reinterpret_cast<char *>(chip::Platform::MemoryAlloc(size));
memcpy(values[i], value, size);
valuesize[i] = size;
Expand All @@ -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
{
Expand All @@ -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];
Expand Down