Convert persistent storage to sync calls and binary data to align wit…

…h KVS style storage (#6286)

* Move async delete to sync delete ... it was mostly sync except darwin and kvs replacement will be sync

* Remove all async usages for persistent storage, move to base64 encoding for what previously was assumed to be null terminated strings

* Remove the storage delegate callback as all methods are sync now

* Restyle fixes

* Remove unused constant definition

* Compilation fixes for darwin

* Restyle fixes

* Undo unintentional line deletion

* Fix persistent storage buffer size handling

* Fix typo

* Fix android compilation errors

* Add back the dispatch into the work queue to make locking work on  darwin

* Update comment stating that storage delegate does not care about data content

* Move android persistent storage to KVS - easier than trying to fix java string conversions

* Restyle fixes

* Remplace async dispatch with sync dispatch in a Sync delegate method

* Fix off by one in python storage delegate implementation

* Remove reference to SendKeyValue callback handler from utils file

* Remove reference to java PersistentStorage

* Remove debug logs from android KVS, make sure JNIEnv getting is thread safe

* Add back KVS operation messages as progress (rather than erro like they were during debugging)

* Remove the AsyncDeleteKeyValue method

* Fix android compilation errors

* Remove duplicate declaration for CHIP_CONTROLLER_QUEUE - fixes darwin build

examples/chip-tool/config/PersistentStorage.cpp

@@ -17,7 +17,10 @@
  */
 #include "PersistentStorage.h"
 
+#include <support/Base64.h>
+
 #include <fstream>
+#include <memory>
 
 using String   = std::basic_string<char>;
 using Section  = std::map<String, String>;
@@ -33,6 +36,37 @@ constexpr const char kPortKey[]            = "ListenPort";
 constexpr const char kLoggingKey[]         = "LoggingLevel";
 constexpr LogCategory kDefaultLoggingLevel = kLogCategory_Detail;
 
+namespace {
+
+std::string StringToBase64(const std::string & value)
+{
+    std::unique_ptr<char[]> buffer(new char[BASE64_ENCODED_LEN(value.length())]);
+
+    uint32_t len =
+        chip::Base64Encode32(reinterpret_cast<const uint8_t *>(value.data()), static_cast<uint32_t>(value.length()), buffer.get());
+    if (len == UINT32_MAX)
+    {
+        return "";
+    }
+
+    return std::string(buffer.get(), len);
+}
+
+std::string Base64ToString(const std::string & b64Value)
+{
+    std::unique_ptr<uint8_t[]> buffer(new uint8_t[BASE64_MAX_DECODED_LEN(b64Value.length())]);
+
+    uint32_t len = chip::Base64Decode32(b64Value.data(), static_cast<uint32_t>(b64Value.length()), buffer.get());
+    if (len == UINT32_MAX)
+    {
+        return "";
+    }
+
+    return std::string(reinterpret_cast<const char *>(buffer.get()), len);
+}
+
+} // namespace
+
 CHIP_ERROR PersistentStorage::Init()
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
@@ -53,46 +87,47 @@ CHIP_ERROR PersistentStorage::Init()
     return err;
 }
 
-void PersistentStorage::SetStorageDelegate(PersistentStorageResultDelegate * delegate) {}
-
-CHIP_ERROR PersistentStorage::SyncGetKeyValue(const char * key, char * value, uint16_t & size)
+CHIP_ERROR PersistentStorage::SyncGetKeyValue(const char * key, void * value, uint16_t & size)
 {
-    CHIP_ERROR err = CHIP_NO_ERROR;
     std::string iniValue;
-    size_t iniValueLength = 0;
 
     auto section = mConfig.sections[kDefaultSectionName];
     auto it      = section.find(key);
-    VerifyOrExit(it != section.end(), err = CHIP_ERROR_KEY_NOT_FOUND);
+    ReturnErrorCodeIf(it == section.end(), CHIP_ERROR_KEY_NOT_FOUND);
+
+    ReturnErrorCodeIf(!inipp::extract(section[key], iniValue), CHIP_ERROR_INVALID_ARGUMENT);
 
-    VerifyOrExit(inipp::extract(section[key], iniValue), err = CHIP_ERROR_INVALID_ARGUMENT);
+    iniValue = Base64ToString(iniValue);
 
-    iniValueLength = iniValue.size();
-    VerifyOrExit(iniValueLength <= static_cast<size_t>(size) - 1, err = CHIP_ERROR_BUFFER_TOO_SMALL);
+    uint16_t dataSize = static_cast<uint16_t>(iniValue.size());
+    if (dataSize > size)
+    {
+        size = dataSize;
+        return CHIP_ERROR_BUFFER_TOO_SMALL;
+    }
 
-    iniValueLength        = iniValue.copy(value, iniValueLength);
-    value[iniValueLength] = '\0';
+    size = dataSize;
+    memcpy(value, iniValue.data(), dataSize);
 
-exit:
-    return err;
+    return CHIP_NO_ERROR;
 }
 
-void PersistentStorage::AsyncSetKeyValue(const char * key, const char * value)
+CHIP_ERROR PersistentStorage::SyncSetKeyValue(const char * key, const void * value, uint16_t size)
 {
     auto section = mConfig.sections[kDefaultSectionName];
-    section[key] = std::string(value);
+    section[key] = StringToBase64(std::string(static_cast<const char *>(value), size));
 
     mConfig.sections[kDefaultSectionName] = section;
-    CommitConfig();
+    return CommitConfig();
 }
 
-void PersistentStorage::AsyncDeleteKeyValue(const char * key)
+CHIP_ERROR PersistentStorage::SyncDeleteKeyValue(const char * key)
 {
     auto section = mConfig.sections[kDefaultSectionName];
     section.erase(key);
 
     mConfig.sections[kDefaultSectionName] = section;
-    CommitConfig();
+    return CommitConfig();
 }
 
 CHIP_ERROR PersistentStorage::CommitConfig()

examples/chip-tool/config/PersistentStorage.h

@@ -28,10 +28,9 @@ class PersistentStorage : public chip::PersistentStorageDelegate
     CHIP_ERROR Init();
 
     /////////// PersistentStorageDelegate Interface /////////
-    void SetStorageDelegate(chip::PersistentStorageResultDelegate * delegate) override;
-    CHIP_ERROR SyncGetKeyValue(const char * key, char * value, uint16_t & size) override;
-    void AsyncSetKeyValue(const char * key, const char * value) override;
-    void AsyncDeleteKeyValue(const char * key) override;
+    CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override;
+    CHIP_ERROR SyncSetKeyValue(const char * key, const void * value, uint16_t size) override;
+    CHIP_ERROR SyncDeleteKeyValue(const char * key) override;
 
     uint16_t GetListenPort();
     chip::Logging::LogCategory GetLoggingLevel();

src/android/CHIPTool/app/src/main/java/com/google/chip/chiptool/CHIPToolActivity.kt

@@ -38,7 +38,6 @@ import com.google.chip.chiptool.setuppayloadscanner.CHIPDeviceDetailsFragment
 import com.google.chip.chiptool.setuppayloadscanner.CHIPDeviceInfo
 import com.google.chip.chiptool.setuppayloadscanner.QrCodeInfo
 import chip.devicecontroller.KeyValueStoreManager
-import chip.devicecontroller.PersistentStorage
 import chip.setuppayload.SetupPayload
 import chip.setuppayload.SetupPayloadParser
 
@@ -54,7 +53,6 @@ class CHIPToolActivity :
     super.onCreate(savedInstanceState)
     setContentView(R.layout.top_activity)
 
-    PersistentStorage.initialize(this);
     KeyValueStoreManager.initialize(this);
 
     if (savedInstanceState == null) {

src/app/server/Server.cpp

@@ -76,18 +76,6 @@ constexpr bool useTestPairing()
 
 class ServerStorageDelegate : public PersistentStorageDelegate
 {
-    void SetStorageDelegate(PersistentStorageResultDelegate * delegate) override
-    {
-        ChipLogError(AppServer, "ServerStorageDelegate does not support async operations");
-        chipDie();
-    }
-
-    void AsyncSetKeyValue(const char * key, const char * value) override
-    {
-        ChipLogError(AppServer, "ServerStorageDelegate does not support async operations");
-        chipDie();
-    }
-
     CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override
     {
         ChipLogProgress(AppServer, "Retrieved value from server storage.");
@@ -105,12 +93,6 @@ class ServerStorageDelegate : public PersistentStorageDelegate
         ChipLogProgress(AppServer, "Delete value in server storage");
         return PersistedStorage::KeyValueStoreMgr().Delete(key);
     }
-
-    void AsyncDeleteKeyValue(const char * key) override
-    {
-        ChipLogProgress(AppServer, "Delete value in server storage.");
-        PersistedStorage::KeyValueStoreMgr().Delete(key);
-    }
 };
 
 ServerStorageDelegate gServerStorage;

src/app/server/StorablePeerConnection.cpp

@@ -50,8 +50,7 @@ CHIP_ERROR StorablePeerConnection::DeleteFromKVS(PersistentStorageDelegate & kvs
     char key[KeySize()];
     ReturnErrorOnFailure(GenerateKey(keyId, key, sizeof(key)));
 
-    kvs.AsyncDeleteKeyValue(key);
-    return CHIP_NO_ERROR;
+    return kvs.SyncDeleteKeyValue(key);
 }
 
 constexpr size_t StorablePeerConnection::KeySize()

src/controller/CHIPDeviceController.cpp

@@ -82,9 +82,6 @@ constexpr uint16_t kMdnsPort = 5353;
 
 constexpr const uint32_t kSessionEstablishmentTimeout = 30 * kMillisecondPerSecond;
 
-// Maximum key ID is 65535 (given it's uint16_t type)
-constexpr uint16_t kMaxKeyIDStringSize = 6;
-
 // This macro generates a key using node ID an key prefix, and performs the given action
 // on that key.
 #define PERSISTENT_KEY_OP(node, keyPrefix, key, action)                                                                            \
@@ -149,11 +146,6 @@ CHIP_ERROR DeviceController::Init(NodeId localDeviceId, ControllerInitParams par
     VerifyOrExit(mBleLayer != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT);
 #endif
 
-    if (mStorageDelegate != nullptr)
-    {
-        mStorageDelegate->SetStorageDelegate(this);
-    }
-
     mTransportMgr = chip::Platform::New<DeviceTransportMgr>();
     mSessionMgr   = chip::Platform::New<SecureSessionMgr>();
     mExchangeMgr  = chip::Platform::New<Messaging::ExchangeManager>();
@@ -228,14 +220,9 @@ CHIP_ERROR DeviceController::Shutdown()
     chip::Platform::Delete(mInetLayer);
 #endif // CONFIG_DEVICE_LAYER
 
-    mSystemLayer = nullptr;
-    mInetLayer   = nullptr;
-
-    if (mStorageDelegate != nullptr)
-    {
-        mStorageDelegate->SetStorageDelegate(nullptr);
-        mStorageDelegate = nullptr;
-    }
+    mSystemLayer     = nullptr;
+    mInetLayer       = nullptr;
+    mStorageDelegate = nullptr;
 
     if (mExchangeMgr != nullptr)
     {
@@ -342,7 +329,7 @@ CHIP_ERROR DeviceController::GetDevice(NodeId deviceId, Device ** out_device)
             uint16_t size = sizeof(deviceInfo.inner);
 
             PERSISTENT_KEY_OP(deviceId, kPairedDeviceKeyPrefix, key,
-                              err = mStorageDelegate->SyncGetKeyValue(key, Uint8::to_char(deviceInfo.inner), size));
+                              err = mStorageDelegate->SyncGetKeyValue(key, deviceInfo.inner, size));
             SuccessOrExit(err);
             VerifyOrExit(size <= sizeof(deviceInfo.inner), err = CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR);
 
@@ -386,8 +373,10 @@ void DeviceController::PersistDevice(Device * device)
     {
         SerializedDevice serialized;
         device->Serialize(serialized);
+
+        // TODO: no need to base-64 the serialized values AGAIN
         PERSISTENT_KEY_OP(device->GetDeviceId(), kPairedDeviceKeyPrefix, key,
-                          mStorageDelegate->AsyncSetKeyValue(key, Uint8::to_const_char(serialized.inner)));
+                          mStorageDelegate->SyncSetKeyValue(key, serialized.inner, sizeof(serialized.inner)));
     }
 }
 
@@ -596,8 +585,6 @@ CHIP_ERROR DeviceController::SetPairedDeviceList(const char * serialized)
     return err;
 }
 
-void DeviceController::OnPersistentStorageStatus(const char * key, Operation op, CHIP_ERROR err) {}
-
 #if CHIP_DEVICE_CONFIG_ENABLE_MDNS
 void DeviceController::OnNodeIdResolved(const chip::Mdns::ResolvedNodeData & nodeData)
 {
@@ -645,25 +632,12 @@ DeviceCommissioner::DeviceCommissioner()
     mPairedDevicesUpdated = false;
 }
 
-CHIP_ERROR DeviceCommissioner::LoadKeyId(PersistentStorageDelegate * delegate, uint16_t & out)
-{
-    VerifyOrReturnError(delegate != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
-
-    // TODO: Consider storing value in binary representation instead of converting to string
-    char keyIDStr[kMaxKeyIDStringSize];
-    uint16_t size = sizeof(keyIDStr);
-    ReturnErrorOnFailure(delegate->SyncGetKeyValue(kNextAvailableKeyID, keyIDStr, size));
-
-    ReturnErrorCodeIf(!ArgParser::ParseInt(keyIDStr, out), CHIP_ERROR_INTERNAL);
-
-    return CHIP_NO_ERROR;
-}
-
 CHIP_ERROR DeviceCommissioner::Init(NodeId localDeviceId, CommissionerInitParams params)
 {
     ReturnErrorOnFailure(DeviceController::Init(localDeviceId, params));
 
-    if (LoadKeyId(mStorageDelegate, mNextKeyId) != CHIP_NO_ERROR)
+    uint16_t size = sizeof(mNextKeyId);
+    if (!mStorageDelegate->SyncGetKeyValue(kNextAvailableKeyID, &mNextKeyId, size) || (size != sizeof(mNextKeyId)))
     {
         mNextKeyId = 0;
     }
@@ -862,7 +836,7 @@ CHIP_ERROR DeviceCommissioner::UnpairDevice(NodeId remoteDeviceId)
 
     if (mStorageDelegate != nullptr)
     {
-        PERSISTENT_KEY_OP(remoteDeviceId, kPairedDeviceKeyPrefix, key, mStorageDelegate->AsyncDeleteKeyValue(key));
+        PERSISTENT_KEY_OP(remoteDeviceId, kPairedDeviceKeyPrefix, key, mStorageDelegate->SyncDeleteKeyValue(key));
     }
 
     mPairedDevices.Remove(remoteDeviceId);
@@ -963,8 +937,9 @@ void DeviceCommissioner::PersistDeviceList()
             const char * value = mPairedDevices.SerializeBase64(serialized, requiredSize);
             if (value != nullptr && requiredSize <= size)
             {
+                // TODO: no need to base64 again the value
                 PERSISTENT_KEY_OP(static_cast<uint64_t>(0), kPairedDeviceListKeyPrefix, key,
-                                  mStorageDelegate->AsyncSetKeyValue(key, value));
+                                  mStorageDelegate->SyncSetKeyValue(key, value, static_cast<uint16_t>(strlen(value))));
                 mPairedDevicesUpdated = false;
             }
             chip::Platform::MemoryFree(serialized);
@@ -976,10 +951,7 @@ void DeviceCommissioner::PersistNextKeyId()
 {
     if (mStorageDelegate != nullptr)
     {
-        // TODO: Consider storing value in binary representation instead of converting to string
-        char keyIDStr[kMaxKeyIDStringSize];
-        snprintf(keyIDStr, sizeof(keyIDStr), "%d", mNextKeyId);
-        mStorageDelegate->AsyncSetKeyValue(kNextAvailableKeyID, keyIDStr);
+        mStorageDelegate->SyncSetKeyValue(kNextAvailableKeyID, &mNextKeyId, sizeof(mNextKeyId));
     }
 }
 

src/controller/CHIPDeviceController.h

@@ -130,7 +130,6 @@ struct CommissionerInitParams : public ControllerInitParams
  */
 class DLL_EXPORT DeviceController : public Messaging::ExchangeDelegate,
                                     public Messaging::ExchangeMgrDelegate,
-                                    public PersistentStorageResultDelegate,
 #if CHIP_DEVICE_CONFIG_ENABLE_MDNS
                                     public Mdns::ResolverDelegate,
 #endif
@@ -260,9 +259,6 @@ class DLL_EXPORT DeviceController : public Messaging::ExchangeDelegate,
     void OnNewConnection(SecureSessionHandle session, Messaging::ExchangeManager * mgr) override;
     void OnConnectionExpired(SecureSessionHandle session, Messaging::ExchangeManager * mgr) override;
 
-    //////////// PersistentStorageResultDelegate Implementation ///////////////
-    void OnPersistentStorageStatus(const char * key, Operation op, CHIP_ERROR err) override;
-
 #if CHIP_DEVICE_CONFIG_ENABLE_MDNS
     //////////// ResolverDelegate Implementation ///////////////
     void OnNodeIdResolved(const chip::Mdns::ResolvedNodeData & nodeData) override;