Change persistent storage SyncGetKeyValue API to match constraints of KvsStorageManager

examples/chip-tool/config/PersistentStorage.cpp

@@ -103,6 +103,8 @@ CHIP_ERROR PersistentStorage::SyncGetKeyValue(const char * key, void * value, ui
 {
     std::string iniValue;
 
+    ReturnErrorCodeIf(((value == nullptr) && (size != 0)), CHIP_ERROR_INVALID_ARGUMENT);
+
     auto section = mConfig.sections[kDefaultSectionName];
     auto it      = section.find(key);
     ReturnErrorCodeIf(it == section.end(), CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
@@ -112,16 +114,14 @@ CHIP_ERROR PersistentStorage::SyncGetKeyValue(const char * key, void * value, ui
     iniValue = Base64ToString(iniValue);
 
     uint16_t dataSize = static_cast<uint16_t>(iniValue.size());
-    if (dataSize > size)
-    {
-        size = dataSize;
-        return CHIP_ERROR_BUFFER_TOO_SMALL;
-    }
+    ReturnErrorCodeIf(size == 0 && dataSize == 0, CHIP_NO_ERROR);
+    ReturnErrorCodeIf(value == nullptr, CHIP_ERROR_BUFFER_TOO_SMALL);
 
-    size = dataSize;
-    memcpy(value, iniValue.data(), dataSize);
+    uint16_t sizeToCopy = std::min(size, dataSize);
 
-    return CHIP_NO_ERROR;
+    memcpy(value, iniValue.data(), sizeToCopy);
+    size = sizeToCopy;
+    return size < dataSize ? CHIP_ERROR_BUFFER_TOO_SMALL : CHIP_NO_ERROR;
 }
 
 CHIP_ERROR PersistentStorage::SyncSetKeyValue(const char * key, const void * value, uint16_t size)

src/controller/python/ChipDeviceController-StorageDelegate.cpp

@@ -24,35 +24,29 @@
 #include <string>
 
 #include <lib/core/CHIPPersistentStorageDelegate.h>
+#include <lib/support/CodeUtils.h>
 #include <lib/support/logging/CHIPLogging.h>
 
 namespace chip {
 namespace Controller {
 
 CHIP_ERROR PythonPersistentStorageDelegate::SyncGetKeyValue(const char * key, void * value, uint16_t & size)
 {
+    ReturnErrorCodeIf(((value == nullptr) && (size != 0)), CHIP_ERROR_INVALID_ARGUMENT);
+
     auto val = mStorage.find(key);
     if (val == mStorage.end())
     {
         return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND;
     }
 
-    if (value == nullptr)
-    {
-        size = 0;
-    }
-
     uint16_t neededSize = val->second.size();
-    if (size == 0)
-    {
-        size = neededSize;
-        return CHIP_ERROR_BUFFER_TOO_SMALL;
-    }
+    ReturnErrorCodeIf(size == 0 && neededSize == 0, CHIP_NO_ERROR);
+    ReturnErrorCodeIf(value == nullptr, CHIP_ERROR_BUFFER_TOO_SMALL);
 
     if (size < neededSize)
     {
         memcpy(value, val->second.data(), size);
-        size = neededSize;
         return CHIP_ERROR_BUFFER_TOO_SMALL;
     }
 
@@ -86,6 +80,10 @@ namespace Python {
 CHIP_ERROR StorageAdapter::SyncGetKeyValue(const char * key, void * value, uint16_t & size)
 {
     ChipLogDetail(Controller, "StorageAdapter::GetKeyValue: Key = %s, Value = %p (%u)", key, value, size);
+    if ((value == nullptr) && (size != 0))
+    {
+        return CHIP_ERROR_INVALID_ARGUMENT;
+    }
 
     uint16_t tmpSize = size;
 
@@ -99,7 +97,6 @@ CHIP_ERROR StorageAdapter::SyncGetKeyValue(const char * key, void * value, uint1
     if (size < tmpSize)
     {
         ChipLogDetail(Controller, "Buf not big enough\n");
-        size = tmpSize;
         return CHIP_ERROR_BUFFER_TOO_SMALL;
     }
 

src/controller/python/chip/storage/__init__.py

@@ -46,23 +46,34 @@ def _OnSyncSetKeyValueCb(storageObj, key: str, value, size):
 
 @_SyncGetKeyValueCbFunct
 def _OnSyncGetKeyValueCb(storageObj, key: str, value, size):
+    ''' This does not adhere to the API requirements of
+    PersistentStorageDelegate::SyncGetKeyValue, but that is okay since
+    the C++ storage binding layer is capable of adapting results from
+    this method to the requirements of
+    PersistentStorageDelegate::SyncGetKeyValue.
+    '''
     try:
         keyValue = storageObj.GetSdkKey(key.decode("utf-8"))
     except Exception as ex:
         keyValue = None
 
     if (keyValue):
-        if (size[0] < len(keyValue)):
-            size[0] = len(keyValue)
-            return
+        sizeOfValue = size[0]
+        sizeToCopy = min(sizeOfValue, len(keyValue))
 
         count = 0
 
         for idx, val in enumerate(keyValue):
+            if sizeToCopy == count:
+                break
             value[idx] = val
             count = count + 1
 
-        size[0] = count
+        # As mentioned above, we are intentionally not returning
+        # sizeToCopy as one might expect because the caller
+        # will use the value in size[0] to determine if it should
+        # return CHIP_ERROR_BUFFER_TOO_SMALL.
+        size[0] = len(keyValue)
     else:
         size[0] = 0
 

src/darwin/Framework/CHIP/CHIPPersistentStorageDelegateBridge.mm

@@ -45,19 +45,17 @@
         }
 
         if ([value length] > UINT16_MAX) {
-            error = CHIP_ERROR_BUFFER_TOO_SMALL;
-            size = 0;
+            error = CHIP_ERROR_PERSISTED_STORAGE_FAILED;
             return;
         }
 
         uint16_t valueSize = static_cast<uint16_t>([value length]);
         if (valueSize > size) {
             error = CHIP_ERROR_BUFFER_TOO_SMALL;
+        } else {
             size = valueSize;
-            return;
         }
 
-        size = valueSize;
         if (size != 0) {
             // buffer is known to be non-null here.
             memcpy(buffer, [value bytes], size);

src/lib/core/CHIPPersistentStorageDelegate.h

@@ -46,30 +46,26 @@ class DLL_EXPORT PersistentStorageDelegate
      *   Caller is responsible to take care of any special formatting needs (e.g. byte
      *   order, null terminators, consistency checks or versioning).
      *
-     *   This API allows for determining the size of a stored value. Whenever
-     *   the passed `size` is smaller than needed and the key exists in storage, the error
-     *   CHIP_ERROR_BUFFER_TOO_SMALL will be given, and the `size` will be updated to the
-     *   size of the stored value. It is legal to use `nullptr` for `buffer` if `size` is 0.
-     *
      *   If a key is found and the `buffer`'s `size` is large enough, then the value will
      *   be copied to `buffer` and `size` will be updated to the actual size used.
      *
-     *   The easiest way to determine if a key exists (and the value's size if so) is to pass
-     *   `size` of 0, which is always valid to do, and will return CHIP_ERROR_BUFFER_TOO_SMALL
-     *   if the key exists and CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if the
-     *   key is not found.
+     *   Whenever the passed `size` is smaller than the size of the stored value for the given key,
+     *   CHIP_ERROR_BUFFER_TOO_SMALL will be returned, but the `buffer` will still be filled with the
+     *   first `size` bytes of the stored value.
+     *
+     *   In the case where `size` of 0 is given, and the value stored is of size 0, CHIP_NO_ERROR is
+     *   returned. It is recommended to use helper method SyncDoesKeyExist(key) to determine if key
+     *   exists.
+     *
+     *   It is legal to use `nullptr` for `buffer` if `size` is 0.
      *
      * @param[in]      key Key to lookup
      * @param[out]     buffer Pointer to a buffer where the place the read value.
-     * @param[in, out] size Input is maximum buffer size, output updated to length of value.
+     * @param[in, out] size Input is maximum buffer size, output updated to number of bytes written into `buffer`.
      *
      * @return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND the key is not found in storage.
      * @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.
+     *                                     the first `size` bytes of the value will be placed in `buffer`.
      */
     virtual CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) = 0;
 
@@ -97,6 +93,23 @@ class DLL_EXPORT PersistentStorageDelegate
      *         or another CHIP_ERROR value from implementation on failure.
      */
     virtual CHIP_ERROR SyncDeleteKeyValue(const char * key) = 0;
+
+    /**
+     * @brief
+     *   Helper function that identifies if a key exists.
+     *
+     *   This may be overridden to provide an implementation that is simpler or more direct.
+     *
+     * @param[in] key Key to check if it exist
+     *
+     * @return true if key exists in storage. It returns false if key does not exist in storage or an internal error arises.
+     */
+    virtual bool SyncDoesKeyExist(const char * key)
+    {
+        uint16_t size  = 0;
+        CHIP_ERROR err = SyncGetKeyValue(key, nullptr, size);
+        return (err == CHIP_ERROR_BUFFER_TOO_SMALL) || (err == CHIP_NO_ERROR);
+    }
 };
 
 } // namespace chip