From 2710b7b35d3f42b701c5f85bf162180a48dfced4 Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Wed, 20 Oct 2021 17:47:41 -0700 Subject: [PATCH 01/10] Separate FabricTable storage from Persistent Storage Delegate --- .../chip-tool/commands/common/CHIPCommand.cpp | 2 + .../chip-tool/commands/common/CHIPCommand.h | 1 + examples/platform/linux/AppMain.cpp | 4 + src/app/server/Server.h | 14 +++- .../CHIPDeviceControllerFactory.cpp | 4 +- src/controller/CHIPDeviceControllerFactory.h | 2 + .../java/AndroidDeviceControllerWrapper.cpp | 17 +++++ .../java/AndroidDeviceControllerWrapper.h | 8 +- .../ChipDeviceController-ScriptBinding.cpp | 5 ++ .../python/chip/internal/CommissionerImpl.cpp | 14 ++-- .../Framework/CHIP/CHIPDeviceController.mm | 13 ++++ .../secure_channel/tests/TestCASESession.cpp | 14 +++- src/transport/FabricTable.cpp | 36 +++++++-- src/transport/FabricTable.h | 73 +++++++++++++++++-- 14 files changed, 186 insertions(+), 21 deletions(-) diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp index 2f2a7449bccc75..273715e7b934ec 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.cpp +++ b/examples/chip-tool/commands/common/CHIPCommand.cpp @@ -37,6 +37,7 @@ CHIP_ERROR CHIPCommand::Run() ReturnLogErrorOnFailure(mStorage.Init()); ReturnLogErrorOnFailure(mOpCredsIssuer.Initialize(mStorage)); + ReturnLogErrorOnFailure(mFabricStorage.Initialize(&mStorage)); chip::Platform::ScopedMemoryBuffer noc; chip::Platform::ScopedMemoryBuffer icac; @@ -64,6 +65,7 @@ CHIP_ERROR CHIPCommand::Run() chip::Controller::FactoryInitParams factoryInitParams; factoryInitParams.storageDelegate = &mStorage; + factoryInitParams.fabricStorage = &mFabricStorage; factoryInitParams.listenPort = mStorage.GetListenPort(); chip::Controller::SetupParams commissionerParams; diff --git a/examples/chip-tool/commands/common/CHIPCommand.h b/examples/chip-tool/commands/common/CHIPCommand.h index eca1a8466e9220..9bb5345e8f47fe 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.h +++ b/examples/chip-tool/commands/common/CHIPCommand.h @@ -65,6 +65,7 @@ class CHIPCommand : public Command ChipDeviceCommissioner mController; PersistentStorage mStorage; + chip::SimpleFabricStorage mFabricStorage; private: static void RunQueuedCommand(intptr_t commandArg); diff --git a/examples/platform/linux/AppMain.cpp b/examples/platform/linux/AppMain.cpp index a514eaca993e37..8aba007772c2da 100644 --- a/examples/platform/linux/AppMain.cpp +++ b/examples/platform/linux/AppMain.cpp @@ -209,6 +209,7 @@ class MyServerStorageDelegate : public PersistentStorageDelegate DeviceCommissioner gCommissioner; MyServerStorageDelegate gServerStorage; +chip::SimpleFabricStorage gFabricStorage; ExampleOperationalCredentialsIssuer gOpCredsIssuer; CHIP_ERROR InitCommissioner() @@ -218,7 +219,10 @@ CHIP_ERROR InitCommissioner() chip::Controller::FactoryInitParams factoryParams; chip::Controller::SetupParams params; + ReturnErrorOnFailure(gFabricStorage.Initialize(&gServerStorage)); + factoryParams.storageDelegate = &gServerStorage; + factoryParams.fabricStorage = &gFabricStorage; // use a different listen port for the commissioner. factoryParams.listenPort = LinuxDeviceOptions::GetInstance().securedCommissionerPort; params.deviceAddressUpdateDelegate = nullptr; diff --git a/src/app/server/Server.h b/src/app/server/Server.h index 968e6dbcf4176c..b61500015b9286 100644 --- a/src/app/server/Server.h +++ b/src/app/server/Server.h @@ -86,7 +86,7 @@ class Server : public Messaging::ExchangeDelegate static Server sServer; - class ServerStorageDelegate : public PersistentStorageDelegate + class ServerStorageDelegate : public PersistentStorageDelegate, public FabricStorage { CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override { @@ -108,6 +108,18 @@ class Server : public Messaging::ExchangeDelegate ChipLogProgress(AppServer, "Deleted from server storage: %s", key); return CHIP_NO_ERROR; } + + CHIP_ERROR SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override + { + return SyncSetKeyValue(key, buffer, size); + }; + + CHIP_ERROR SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override + { + return SyncGetKeyValue(key, buffer, size); + }; + + CHIP_ERROR SyncDelete(FabricIndex fabricIndex, const char * key) override { return SyncDeleteKeyValue(key); }; }; // Messaging::ExchangeDelegate diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 43d629a860e4dd..ec8e4f1b5f6686 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -51,6 +51,7 @@ CHIP_ERROR DeviceControllerFactory::Init(FactoryInitParams params) mListenPort = params.listenPort; mStorageDelegate = params.storageDelegate; + mFabricStorage = params.fabricStorage; CHIP_ERROR err = InitSystemState(params); @@ -134,7 +135,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) stateParams.exchangeMgr = chip::Platform::New(); stateParams.messageCounterManager = chip::Platform::New(); - ReturnErrorOnFailure(stateParams.fabricTable->Init(mStorageDelegate)); + ReturnErrorOnFailure(stateParams.fabricTable->Init(mFabricStorage)); ReturnErrorOnFailure( stateParams.sessionMgr->Init(stateParams.systemLayer, stateParams.transportMgr, stateParams.messageCounterManager)); ReturnErrorOnFailure(stateParams.exchangeMgr->Init(stateParams.sessionMgr)); @@ -211,6 +212,7 @@ DeviceControllerFactory::~DeviceControllerFactory() mSystemState = nullptr; } mStorageDelegate = nullptr; + mFabricStorage = nullptr; } CHIP_ERROR DeviceControllerSystemState::Shutdown() diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index a84fd2ca868df7..41ca8685705b96 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -65,6 +65,7 @@ struct SetupParams struct FactoryInitParams { PersistentStorageDelegate * storageDelegate = nullptr; + FabricStorage * fabricStorage = nullptr; System::Layer * systemLayer = nullptr; Inet::InetLayer * inetLayer = nullptr; DeviceControllerInteractionModelDelegate * imDelegate = nullptr; @@ -110,6 +111,7 @@ class DeviceControllerFactory uint16_t mListenPort; PersistentStorageDelegate * mStorageDelegate = nullptr; + FabricStorage * mFabricStorage = nullptr; DeviceControllerSystemState * mSystemState = nullptr; }; diff --git a/src/controller/java/AndroidDeviceControllerWrapper.cpp b/src/controller/java/AndroidDeviceControllerWrapper.cpp index 3344fc7c5729b8..93c32830647bec 100644 --- a/src/controller/java/AndroidDeviceControllerWrapper.cpp +++ b/src/controller/java/AndroidDeviceControllerWrapper.cpp @@ -352,3 +352,20 @@ CHIP_ERROR AndroidDeviceControllerWrapper::SyncDeleteKeyValue(const char * key) ChipLogProgress(chipTool, "KVS: Deleting key %s", key); return chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Delete(key); } + +CHIP_ERROR AndroidDeviceControllerWrapper::SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, + uint16_t size) override +{ + return SyncSetKeyValue(key, buffer, size); +}; + +CHIP_ERROR AndroidDeviceControllerWrapper::SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, + uint16_t & size) override +{ + return SyncGetKeyValue(key, buffer, size); +}; + +CHIP_ERROR AndroidDeviceControllerWrapper::SyncDelete(FabricIndex fabricIndex, const char * key) override +{ + return SyncDeleteKeyValue(key); +}; diff --git a/src/controller/java/AndroidDeviceControllerWrapper.h b/src/controller/java/AndroidDeviceControllerWrapper.h index fc926e1d27e76a..19519e24e5f43d 100644 --- a/src/controller/java/AndroidDeviceControllerWrapper.h +++ b/src/controller/java/AndroidDeviceControllerWrapper.h @@ -37,7 +37,8 @@ class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDelegate, public chip::Controller::DeviceStatusDelegate, public chip::Controller::OperationalCredentialsDelegate, - public chip::PersistentStorageDelegate + public chip::PersistentStorageDelegate, + public chip::FabricStorage { public: ~AndroidDeviceControllerWrapper(); @@ -78,6 +79,11 @@ class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDel CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override; CHIP_ERROR SyncDeleteKeyValue(const char * key) override; + // FabricStorage implementation + CHIP_ERROR SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override; + CHIP_ERROR SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override; + CHIP_ERROR SyncDelete(FabricIndex fabricIndex, const char * key) override; + static AndroidDeviceControllerWrapper * FromJNIHandle(jlong handle) { return reinterpret_cast(handle); diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index 106dfcbd2a9866..ed2353aca200c4 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -86,6 +86,7 @@ chip::Controller::PythonPersistentStorageDelegate sStorageDelegate; chip::Controller::ScriptDevicePairingDelegate sPairingDelegate; chip::Controller::ScriptDeviceAddressUpdateDelegate sDeviceAddressUpdateDelegate; chip::Controller::ExampleOperationalCredentialsIssuer sOperationalCredentialsIssuer; +chip::SimpleFabricStorage sFabricStorage; } // namespace // NOTE: Remote device ID is in sync with the echo server device id @@ -186,6 +187,9 @@ ChipError::StorageType pychip_DeviceController_NewDeviceController(chip::Control CHIP_ERROR err = sOperationalCredentialsIssuer.Initialize(sStorageDelegate); VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger()); + err = sFabricStorage.Initialize(&sStorageDelegate); + VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger()); + chip::Crypto::P256Keypair ephemeralKey; err = ephemeralKey.Initialize(); VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger()); @@ -208,6 +212,7 @@ ChipError::StorageType pychip_DeviceController_NewDeviceController(chip::Control FactoryInitParams factoryParams; factoryParams.storageDelegate = &sStorageDelegate; + factoryParams.fabricStorage = &sFabricStorage; factoryParams.imDelegate = &PythonInteractionModelDelegate::Instance(); SetupParams initParams; diff --git a/src/controller/python/chip/internal/CommissionerImpl.cpp b/src/controller/python/chip/internal/CommissionerImpl.cpp index ac71588061cc8a..aa86cdfad31f67 100644 --- a/src/controller/python/chip/internal/CommissionerImpl.cpp +++ b/src/controller/python/chip/internal/CommissionerImpl.cpp @@ -79,6 +79,7 @@ class ScriptDevicePairingDelegate final : public chip::Controller::DevicePairing }; ServerStorageDelegate gServerStorage; +chip::SimpleFabricStorage gFabricStorage; ScriptDevicePairingDelegate gPairingDelegate; chip::Controller::ExampleOperationalCredentialsIssuer gOperationalCredentialsIssuer; @@ -102,18 +103,21 @@ extern "C" chip::Controller::DeviceCommissioner * pychip_internal_Commissioner_N // already assumed initialized chip::Controller::SetupParams commissionerParams; chip::Controller::FactoryInitParams factoryParams; - - commissionerParams.pairingDelegate = &gPairingDelegate; - factoryParams.storageDelegate = &gServerStorage; - chip::Platform::ScopedMemoryBuffer noc; chip::Platform::ScopedMemoryBuffer icac; chip::Platform::ScopedMemoryBuffer rcac; + chip::Crypto::P256Keypair ephemeralKey; + + err = gFabricStorage.Initialize(&gServerStorage); + SuccessOrExit(err); + + commissionerParams.pairingDelegate = &gPairingDelegate; + factoryParams.storageDelegate = &gServerStorage; + factoryParams.fabricStorage = &gFabricStorage; // Initialize device attestation verifier chip::Credentials::SetDeviceAttestationVerifier(chip::Credentials::Examples::GetExampleDACVerifier()); - chip::Crypto::P256Keypair ephemeralKey; err = ephemeralKey.Initialize(); SuccessOrExit(err); diff --git a/src/darwin/Framework/CHIP/CHIPDeviceController.mm b/src/darwin/Framework/CHIP/CHIPDeviceController.mm index 221e3c037aa41f..bb182bcdaeefb5 100644 --- a/src/darwin/Framework/CHIP/CHIPDeviceController.mm +++ b/src/darwin/Framework/CHIP/CHIPDeviceController.mm @@ -60,6 +60,7 @@ @interface CHIPDeviceController () @property (readonly) chip::Controller::DeviceCommissioner * cppCommissioner; @property (readonly) CHIPDevicePairingDelegateBridge * pairingDelegateBridge; @property (readonly) CHIPPersistentStorageDelegateBridge * persistentStorageDelegateBridge; +@property (readonly) chip::FabricStorage * fabricStorage; @property (readonly) CHIPOperationalCredentialsDelegate * operationalCredentialsDelegate; @property (readonly) CHIPP256KeypairBridge keypairBridge; @property (readonly) chip::NodeId localDeviceId; @@ -124,6 +125,10 @@ - (BOOL)shutdown delete self->_cppCommissioner; self->_cppCommissioner = nullptr; } + if (self->_fabricStorage) { + delete self->_fabricStorage; + self->_fabricStorage = nullptr; + } }); // StopEventLoopTask will block until blocks are executed @@ -153,6 +158,8 @@ - (BOOL)startup:(_Nullable id)storageDelegate CHIP_ERROR errorCode = CHIP_ERROR_INCORRECT_STATE; _persistentStorageDelegateBridge->setFrameworkDelegate(storageDelegate); + // TODO Expose FabricStorage to CHIPFramework consumers. + _fabricStorage = new chip::SimpleFabricStorage(_persistentStorageDelegateBridge); // create a CHIPP256KeypairBridge here and pass it to the operationalCredentialsDelegate std::unique_ptr nativeBridge; @@ -184,6 +191,7 @@ - (BOOL)startup:(_Nullable id)storageDelegate chip::Credentials::SetDeviceAttestationVerifier(chip::Credentials::Examples::GetExampleDACVerifier()); params.storageDelegate = _persistentStorageDelegateBridge; + params.fabricStorage = _fabricStorage; commissionerParams.deviceAddressUpdateDelegate = _pairingDelegateBridge; commissionerParams.pairingDelegate = _pairingDelegateBridge; @@ -474,6 +482,11 @@ - (BOOL)checkForInitError:(BOOL)condition logMsg:(NSString *)logMsg _persistentStorageDelegateBridge = NULL; } + if (_fabricStorage) { + delete _fabricStorage; + _fabricStorage = nullptr; + } + return YES; } diff --git a/src/protocols/secure_channel/tests/TestCASESession.cpp b/src/protocols/secure_channel/tests/TestCASESession.cpp index d249fce013244f..15937f60ba4b33 100644 --- a/src/protocols/secure_channel/tests/TestCASESession.cpp +++ b/src/protocols/secure_channel/tests/TestCASESession.cpp @@ -251,7 +251,7 @@ void CASE_SecurePairingHandshakeTest(nlTestSuite * inSuite, void * inContext) CASE_SecurePairingHandshakeTestCommon(inSuite, inContext, pairingCommissioner, delegateCommissioner); } -class TestPersistentStorageDelegate : public PersistentStorageDelegate +class TestPersistentStorageDelegate : public PersistentStorageDelegate, public FabricStorage { public: TestPersistentStorageDelegate() @@ -319,6 +319,18 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate CHIP_ERROR SyncDeleteKeyValue(const char * key) override { return CHIP_NO_ERROR; } + CHIP_ERROR SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override + { + return SyncSetKeyValue(key, buffer, size); + }; + + CHIP_ERROR SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override + { + return SyncGetKeyValue(key, buffer, size); + }; + + CHIP_ERROR SyncDelete(FabricIndex fabricIndex, const char * key) override { return SyncDeleteKeyValue(key); }; + private: char * keys[16]; void * values[16]; diff --git a/src/transport/FabricTable.cpp b/src/transport/FabricTable.cpp index 5a95024980a859..31ad42cc1c8b6f 100644 --- a/src/transport/FabricTable.cpp +++ b/src/transport/FabricTable.cpp @@ -39,7 +39,7 @@ CHIP_ERROR FabricInfo::SetFabricLabel(const CharSpan & fabricLabel) return CHIP_NO_ERROR; } -CHIP_ERROR FabricInfo::StoreIntoKVS(PersistentStorageDelegate * kvs) +CHIP_ERROR FabricInfo::StoreIntoKVS(FabricStorage * kvs) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -103,7 +103,7 @@ CHIP_ERROR FabricInfo::StoreIntoKVS(PersistentStorageDelegate * kvs) memcpy(info->mNOCCert, mNOCCert.data(), mNOCCert.size()); } - err = kvs->SyncSetKeyValue(key, info, sizeof(StorableFabricInfo)); + err = kvs->SyncStore(mFabric, key, info, sizeof(StorableFabricInfo)); if (err != CHIP_NO_ERROR) { ChipLogError(Discovery, "Error occurred calling SyncSetKeyValue: %s", chip::ErrorStr(err)); @@ -117,7 +117,7 @@ CHIP_ERROR FabricInfo::StoreIntoKVS(PersistentStorageDelegate * kvs) return err; } -CHIP_ERROR FabricInfo::FetchFromKVS(PersistentStorageDelegate * kvs) +CHIP_ERROR FabricInfo::FetchFromKVS(FabricStorage * kvs) { CHIP_ERROR err = CHIP_NO_ERROR; char key[kKeySize]; @@ -134,7 +134,7 @@ CHIP_ERROR FabricInfo::FetchFromKVS(PersistentStorageDelegate * kvs) NodeId nodeId; - SuccessOrExit(err = kvs->SyncGetKeyValue(key, info, infoSize)); + SuccessOrExit(err = kvs->SyncLoad(mFabric, key, info, infoSize)); mFabricId = Encoding::LittleEndian::HostSwap64(info->mFabricId); nodeId = Encoding::LittleEndian::HostSwap64(info->mNodeId); @@ -197,14 +197,14 @@ CHIP_ERROR FabricInfo::GetCompressedId(FabricId fabricId, NodeId nodeId, PeerId return CHIP_NO_ERROR; } -CHIP_ERROR FabricInfo::DeleteFromKVS(PersistentStorageDelegate * kvs, FabricIndex id) +CHIP_ERROR FabricInfo::DeleteFromKVS(FabricStorage * kvs, FabricIndex id) { CHIP_ERROR err = CHIP_NO_ERROR; char key[kKeySize]; ReturnErrorOnFailure(GenerateKey(id, key, sizeof(key))); - err = kvs->SyncDeleteKeyValue(key); + err = kvs->SyncDelete(id, key); if (err != CHIP_NO_ERROR) { ChipLogDetail(Discovery, "Fabric %d is not yet configured", id); @@ -382,6 +382,28 @@ FabricInfo * FabricTable::FindFabricWithIndex(FabricIndex fabricIndex) return nullptr; } +FabricInfo * FabricTable::FindFabricWithCompressedId(CompressedFabricId fabricId) +{ + if (fabricId == kUndefinedCompressedFabricId) + { + return nullptr; + } + + static_assert(kMaxValidFabricIndex <= UINT8_MAX, "Cannot create more fabrics than UINT8_MAX"); + for (FabricIndex i = kMinValidFabricIndex; i <= kMaxValidFabricIndex; i++) + { + FabricInfo * fabric = FindFabricWithIndex(i); + + if (fabric != nullptr && fabricId == fabric->GetPeerId().GetCompressedFabricId()) + { + LoadFromStorage(fabric); + return fabric; + } + } + return nullptr; +} + + void FabricTable::Reset() { static_assert(kMaxValidFabricIndex <= UINT8_MAX, "Cannot create more fabrics than UINT8_MAX"); @@ -554,7 +576,7 @@ void FabricTable::DeleteAllFabrics() } } -CHIP_ERROR FabricTable::Init(PersistentStorageDelegate * storage) +CHIP_ERROR FabricTable::Init(FabricStorage * storage) { VerifyOrReturnError(storage != nullptr, CHIP_ERROR_INVALID_ARGUMENT); mStorage = storage; diff --git a/src/transport/FabricTable.h b/src/transport/FabricTable.h index 47a4ec3fce353d..919cc3fefa77bb 100644 --- a/src/transport/FabricTable.h +++ b/src/transport/FabricTable.h @@ -48,6 +48,68 @@ static constexpr uint8_t kFabricLabelMaxLengthInBytes = 32; constexpr char kFabricTableKeyPrefix[] = "Fabric"; constexpr char kFabricTableCountKey[] = "NumFabrics"; +class DLL_EXPORT FabricStorage +{ +public: + virtual ~FabricStorage() {} + + /** + * Gets called when fabric data needs to be stored. + **/ + virtual CHIP_ERROR SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) = 0; + + /** + * Gets called when fabric data needs to be loaded. + **/ + virtual CHIP_ERROR SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) = 0; + + /** + * Gets called when fabric data needs to be removed. + **/ + virtual CHIP_ERROR SyncDelete(FabricIndex fabricIndex, const char * key) = 0; +}; + +/** + * @brief A default implementation of Fabric storage that preserves legacy behavior of using + * the Persistent storage delegate directly. + * + */ +class DLL_EXPORT SimpleFabricStorage : public FabricStorage +{ +public: + SimpleFabricStorage(){}; + SimpleFabricStorage(PersistentStorageDelegate * storage) : mStorage(storage){}; + ~SimpleFabricStorage() override { mStorage = nullptr; }; + + CHIP_ERROR Initialize(PersistentStorageDelegate * storage) + { + VerifyOrReturnError(mStorage == nullptr || storage == mStorage, CHIP_ERROR_INCORRECT_STATE); + mStorage = storage; + return CHIP_NO_ERROR; + } + + CHIP_ERROR SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override + { + VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); + return mStorage->SyncSetKeyValue(key, buffer, size); + }; + + CHIP_ERROR SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override + { + VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); + return mStorage->SyncGetKeyValue(key, buffer, size); + }; + + CHIP_ERROR SyncDelete(FabricIndex fabricIndex, const char * key) override + { + VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); + return mStorage->SyncDeleteKeyValue(key); + }; + +private: + PersistentStorageDelegate * mStorage = nullptr; +}; + /** * Defines state of a pairing established by a fabric. * Node ID is only settable using the device operational credentials. @@ -209,9 +271,9 @@ class DLL_EXPORT FabricInfo static CHIP_ERROR GenerateKey(FabricIndex id, char * key, size_t len); - CHIP_ERROR StoreIntoKVS(PersistentStorageDelegate * kvs); - CHIP_ERROR FetchFromKVS(PersistentStorageDelegate * kvs); - static CHIP_ERROR DeleteFromKVS(PersistentStorageDelegate * kvs, FabricIndex id); + CHIP_ERROR StoreIntoKVS(FabricStorage * kvs); + CHIP_ERROR FetchFromKVS(FabricStorage * kvs); + static CHIP_ERROR DeleteFromKVS(FabricStorage * kvs, FabricIndex id); void ReleaseCert(MutableByteSpan & cert); void ReleaseOperationalCerts() @@ -356,13 +418,14 @@ class DLL_EXPORT FabricTable void ReleaseFabricIndex(FabricIndex fabricIndex); FabricInfo * FindFabricWithIndex(FabricIndex fabricIndex); + FabricInfo * FindFabricWithCompressedId(CompressedFabricId fabricId); FabricIndex FindDestinationIDCandidate(const ByteSpan & destinationId, const ByteSpan & initiatorRandom, const ByteSpan * ipkList, size_t ipkListEntries); void Reset(); - CHIP_ERROR Init(PersistentStorageDelegate * storage); + CHIP_ERROR Init(FabricStorage * storage); CHIP_ERROR SetFabricDelegate(FabricTableDelegate * delegate); uint8_t FabricCount() const { return mFabricCount; } @@ -377,7 +440,7 @@ class DLL_EXPORT FabricTable private: FabricInfo mStates[CHIP_CONFIG_MAX_DEVICE_ADMINS]; - PersistentStorageDelegate * mStorage = nullptr; + FabricStorage * mStorage = nullptr; // TODO: Fabric table should be backed by a single backing store (attribute store), remove delegate callbacks #6419 FabricTableDelegate * mDelegate = nullptr; From 17ad7e35e93f95e4f8d8142c11e9afa43e1226ff Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Wed, 20 Oct 2021 18:19:35 -0700 Subject: [PATCH 02/10] Fix Android compile --- src/controller/java/AndroidDeviceControllerWrapper.cpp | 6 +++--- src/controller/java/AndroidDeviceControllerWrapper.h | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/controller/java/AndroidDeviceControllerWrapper.cpp b/src/controller/java/AndroidDeviceControllerWrapper.cpp index 93c32830647bec..b805d32ec64ad8 100644 --- a/src/controller/java/AndroidDeviceControllerWrapper.cpp +++ b/src/controller/java/AndroidDeviceControllerWrapper.cpp @@ -353,19 +353,19 @@ CHIP_ERROR AndroidDeviceControllerWrapper::SyncDeleteKeyValue(const char * key) return chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Delete(key); } -CHIP_ERROR AndroidDeviceControllerWrapper::SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, +CHIP_ERROR AndroidDeviceControllerWrapper::SyncStore(chip::FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override { return SyncSetKeyValue(key, buffer, size); }; -CHIP_ERROR AndroidDeviceControllerWrapper::SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, +CHIP_ERROR AndroidDeviceControllerWrapper::SyncLoad(chip::FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override { return SyncGetKeyValue(key, buffer, size); }; -CHIP_ERROR AndroidDeviceControllerWrapper::SyncDelete(FabricIndex fabricIndex, const char * key) override +CHIP_ERROR AndroidDeviceControllerWrapper::SyncDelete(chip::FabricIndex fabricIndex, const char * key) override { return SyncDeleteKeyValue(key); }; diff --git a/src/controller/java/AndroidDeviceControllerWrapper.h b/src/controller/java/AndroidDeviceControllerWrapper.h index 19519e24e5f43d..3c1ef3f663596e 100644 --- a/src/controller/java/AndroidDeviceControllerWrapper.h +++ b/src/controller/java/AndroidDeviceControllerWrapper.h @@ -80,9 +80,9 @@ class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDel CHIP_ERROR SyncDeleteKeyValue(const char * key) override; // FabricStorage implementation - CHIP_ERROR SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override; - CHIP_ERROR SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override; - CHIP_ERROR SyncDelete(FabricIndex fabricIndex, const char * key) override; + CHIP_ERROR SyncStore(chip::FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override; + CHIP_ERROR SyncLoad(chip::FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override; + CHIP_ERROR SyncDelete(chip::FabricIndex fabricIndex, const char * key) override; static AndroidDeviceControllerWrapper * FromJNIHandle(jlong handle) { From 37c5230e665210243458abe2edab5a11560b8e14 Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Wed, 20 Oct 2021 18:24:20 -0700 Subject: [PATCH 03/10] Fix format --- src/transport/FabricTable.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/transport/FabricTable.cpp b/src/transport/FabricTable.cpp index 31ad42cc1c8b6f..74bbe230469d20 100644 --- a/src/transport/FabricTable.cpp +++ b/src/transport/FabricTable.cpp @@ -403,7 +403,6 @@ FabricInfo * FabricTable::FindFabricWithCompressedId(CompressedFabricId fabricId return nullptr; } - void FabricTable::Reset() { static_assert(kMaxValidFabricIndex <= UINT8_MAX, "Cannot create more fabrics than UINT8_MAX"); From a61fc00816da0b99230bf04e0609d22ac1ce7fdb Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Wed, 20 Oct 2021 19:47:38 -0700 Subject: [PATCH 04/10] Attempt to fix compile again --- src/controller/java/AndroidDeviceControllerWrapper.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/controller/java/AndroidDeviceControllerWrapper.cpp b/src/controller/java/AndroidDeviceControllerWrapper.cpp index b805d32ec64ad8..a7ed501fe30c67 100644 --- a/src/controller/java/AndroidDeviceControllerWrapper.cpp +++ b/src/controller/java/AndroidDeviceControllerWrapper.cpp @@ -354,18 +354,17 @@ CHIP_ERROR AndroidDeviceControllerWrapper::SyncDeleteKeyValue(const char * key) } CHIP_ERROR AndroidDeviceControllerWrapper::SyncStore(chip::FabricIndex fabricIndex, const char * key, const void * buffer, - uint16_t size) override + uint16_t size) { return SyncSetKeyValue(key, buffer, size); }; -CHIP_ERROR AndroidDeviceControllerWrapper::SyncLoad(chip::FabricIndex fabricIndex, const char * key, void * buffer, - uint16_t & size) override +CHIP_ERROR AndroidDeviceControllerWrapper::SyncLoad(chip::FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) { return SyncGetKeyValue(key, buffer, size); }; -CHIP_ERROR AndroidDeviceControllerWrapper::SyncDelete(chip::FabricIndex fabricIndex, const char * key) override +CHIP_ERROR AndroidDeviceControllerWrapper::SyncDelete(chip::FabricIndex fabricIndex, const char * key) { return SyncDeleteKeyValue(key); }; From a08d3ca6d5f02467b5eb818393a6fc0187ef3e06 Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Wed, 20 Oct 2021 23:22:45 -0700 Subject: [PATCH 05/10] Move the persistent storage delegate into the Controller/Commissioner SetupParams --- .../chip-tool/commands/common/CHIPCommand.cpp | 6 ++-- examples/platform/linux/AppMain.cpp | 4 +-- .../CHIPDeviceControllerFactory.cpp | 10 +++---- src/controller/CHIPDeviceControllerFactory.h | 10 +++---- .../java/AndroidDeviceControllerWrapper.cpp | 7 +++-- .../ChipDeviceController-ScriptBinding.cpp | 6 ++-- .../python/chip/internal/CommissionerImpl.cpp | 5 ++-- .../Framework/CHIP/CHIPDeviceController.mm | 28 +++++++++++-------- 8 files changed, 41 insertions(+), 35 deletions(-) diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp index 273715e7b934ec..6f204c99d3d799 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.cpp +++ b/examples/chip-tool/commands/common/CHIPCommand.cpp @@ -64,11 +64,11 @@ CHIP_ERROR CHIPCommand::Run() rcacSpan, icacSpan, nocSpan)); chip::Controller::FactoryInitParams factoryInitParams; - factoryInitParams.storageDelegate = &mStorage; - factoryInitParams.fabricStorage = &mFabricStorage; - factoryInitParams.listenPort = mStorage.GetListenPort(); + factoryInitParams.fabricStorage = &mFabricStorage; + factoryInitParams.listenPort = mStorage.GetListenPort(); chip::Controller::SetupParams commissionerParams; + commissionerParams.storageDelegate = &mStorage; commissionerParams.operationalCredentialsDelegate = &mOpCredsIssuer; commissionerParams.ephemeralKeypair = &ephemeralKey; commissionerParams.controllerRCAC = rcacSpan; diff --git a/examples/platform/linux/AppMain.cpp b/examples/platform/linux/AppMain.cpp index 8aba007772c2da..9a249b56c92daf 100644 --- a/examples/platform/linux/AppMain.cpp +++ b/examples/platform/linux/AppMain.cpp @@ -221,10 +221,10 @@ CHIP_ERROR InitCommissioner() ReturnErrorOnFailure(gFabricStorage.Initialize(&gServerStorage)); - factoryParams.storageDelegate = &gServerStorage; - factoryParams.fabricStorage = &gFabricStorage; + factoryParams.fabricStorage = &gFabricStorage; // use a different listen port for the commissioner. factoryParams.listenPort = LinuxDeviceOptions::GetInstance().securedCommissionerPort; + params.storageDelegate = &gServerStorage; params.deviceAddressUpdateDelegate = nullptr; params.operationalCredentialsDelegate = &gOpCredsIssuer; diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index ec8e4f1b5f6686..a15c5940b186c9 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -49,9 +49,8 @@ CHIP_ERROR DeviceControllerFactory::Init(FactoryInitParams params) return CHIP_NO_ERROR; } - mListenPort = params.listenPort; - mStorageDelegate = params.storageDelegate; - mFabricStorage = params.fabricStorage; + mListenPort = params.listenPort; + mFabricStorage = params.fabricStorage; CHIP_ERROR err = InitSystemState(params); @@ -161,9 +160,9 @@ void DeviceControllerFactory::PopulateInitParams(ControllerInitParams & controll controllerParams.controllerICAC = params.controllerICAC; controllerParams.controllerRCAC = params.controllerRCAC; controllerParams.fabricId = params.fabricId; + controllerParams.storageDelegate = params.storageDelegate; controllerParams.systemState = mSystemState; - controllerParams.storageDelegate = mStorageDelegate; controllerParams.controllerVendorId = params.controllerVendorId; } @@ -211,8 +210,7 @@ DeviceControllerFactory::~DeviceControllerFactory() chip::Platform::Delete(mSystemState); mSystemState = nullptr; } - mStorageDelegate = nullptr; - mFabricStorage = nullptr; + mFabricStorage = nullptr; } CHIP_ERROR DeviceControllerSystemState::Shutdown() diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index 41ca8685705b96..3c84e5e5b41c5e 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -43,6 +43,8 @@ struct SetupParams #endif OperationalCredentialsDelegate * operationalCredentialsDelegate = nullptr; + PersistentStorageDelegate * storageDelegate = nullptr; + /* The following keypair must correspond to the public key used for generating controllerNOC. It's used by controller to establish CASE sessions with devices */ Crypto::P256Keypair * ephemeralKeypair = nullptr; @@ -60,11 +62,10 @@ struct SetupParams DevicePairingDelegate * pairingDelegate = nullptr; }; -// TODO everything other than the storage delegate here should be removed. +// TODO everything other than the fabric storage here should be removed. // We're blocked because of the need to support !CHIP_DEVICE_LAYER struct FactoryInitParams { - PersistentStorageDelegate * storageDelegate = nullptr; FabricStorage * fabricStorage = nullptr; System::Layer * systemLayer = nullptr; Inet::InetLayer * inetLayer = nullptr; @@ -110,9 +111,8 @@ class DeviceControllerFactory CHIP_ERROR InitSystemState(); uint16_t mListenPort; - PersistentStorageDelegate * mStorageDelegate = nullptr; - FabricStorage * mFabricStorage = nullptr; - DeviceControllerSystemState * mSystemState = nullptr; + FabricStorage * mFabricStorage = nullptr; + DeviceControllerSystemState * mSystemState = nullptr; }; } // namespace Controller diff --git a/src/controller/java/AndroidDeviceControllerWrapper.cpp b/src/controller/java/AndroidDeviceControllerWrapper.cpp index a7ed501fe30c67..d293fb0ace44b5 100644 --- a/src/controller/java/AndroidDeviceControllerWrapper.cpp +++ b/src/controller/java/AndroidDeviceControllerWrapper.cpp @@ -209,12 +209,13 @@ AndroidDeviceControllerWrapper * AndroidDeviceControllerWrapper::AllocateNew(Jav chip::Controller::FactoryInitParams initParams; chip::Controller::SetupParams setupParams; - initParams.storageDelegate = wrapper.get(); - initParams.systemLayer = systemLayer; - initParams.inetLayer = inetLayer; + initParams.systemLayer = systemLayer; + initParams.inetLayer = inetLayer; + initParams.fabricStorage = wrapper.get(); // move bleLayer into platform/android to share with app server initParams.bleLayer = DeviceLayer::ConnectivityMgr().GetBleLayer(); initParams.listenPort = CHIP_PORT + 1; + setupParams.storageDelegate = wrapper.get(); setupParams.pairingDelegate = wrapper.get(); setupParams.operationalCredentialsDelegate = wrapper.get(); diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index ed2353aca200c4..d94d8a9cfbf464 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -211,11 +211,11 @@ ChipError::StorageType pychip_DeviceController_NewDeviceController(chip::Control VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger()); FactoryInitParams factoryParams; - factoryParams.storageDelegate = &sStorageDelegate; - factoryParams.fabricStorage = &sFabricStorage; - factoryParams.imDelegate = &PythonInteractionModelDelegate::Instance(); + factoryParams.fabricStorage = &sFabricStorage; + factoryParams.imDelegate = &PythonInteractionModelDelegate::Instance(); SetupParams initParams; + initParams.storageDelegate = &sStorageDelegate; initParams.deviceAddressUpdateDelegate = &sDeviceAddressUpdateDelegate; initParams.pairingDelegate = &sPairingDelegate; initParams.operationalCredentialsDelegate = &sOperationalCredentialsIssuer; diff --git a/src/controller/python/chip/internal/CommissionerImpl.cpp b/src/controller/python/chip/internal/CommissionerImpl.cpp index aa86cdfad31f67..1f718adbdf0270 100644 --- a/src/controller/python/chip/internal/CommissionerImpl.cpp +++ b/src/controller/python/chip/internal/CommissionerImpl.cpp @@ -111,9 +111,10 @@ extern "C" chip::Controller::DeviceCommissioner * pychip_internal_Commissioner_N err = gFabricStorage.Initialize(&gServerStorage); SuccessOrExit(err); + factoryParams.fabricStorage = &gFabricStorage; + commissionerParams.pairingDelegate = &gPairingDelegate; - factoryParams.storageDelegate = &gServerStorage; - factoryParams.fabricStorage = &gFabricStorage; + commissionerParams.storageDelegate = &gServerStorage; // Initialize device attestation verifier chip::Credentials::SetDeviceAttestationVerifier(chip::Credentials::Examples::GetExampleDACVerifier()); diff --git a/src/darwin/Framework/CHIP/CHIPDeviceController.mm b/src/darwin/Framework/CHIP/CHIPDeviceController.mm index bb182bcdaeefb5..5abb3edee538f4 100644 --- a/src/darwin/Framework/CHIP/CHIPDeviceController.mm +++ b/src/darwin/Framework/CHIP/CHIPDeviceController.mm @@ -125,10 +125,6 @@ - (BOOL)shutdown delete self->_cppCommissioner; self->_cppCommissioner = nullptr; } - if (self->_fabricStorage) { - delete self->_fabricStorage; - self->_fabricStorage = nullptr; - } }); // StopEventLoopTask will block until blocks are executed @@ -160,7 +156,9 @@ - (BOOL)startup:(_Nullable id)storageDelegate _persistentStorageDelegateBridge->setFrameworkDelegate(storageDelegate); // TODO Expose FabricStorage to CHIPFramework consumers. _fabricStorage = new chip::SimpleFabricStorage(_persistentStorageDelegateBridge); - + if ([self checkForStartError:(_fabricStorage != nullptr) logMsg:kErrorMemoryInit]) { + return; + } // create a CHIPP256KeypairBridge here and pass it to the operationalCredentialsDelegate std::unique_ptr nativeBridge; if (nocSigner != nil) { @@ -190,8 +188,8 @@ - (BOOL)startup:(_Nullable id)storageDelegate // Initialize device attestation verifier chip::Credentials::SetDeviceAttestationVerifier(chip::Credentials::Examples::GetExampleDACVerifier()); - params.storageDelegate = _persistentStorageDelegateBridge; params.fabricStorage = _fabricStorage; + commissionerParams.storageDelegate = _persistentStorageDelegateBridge; commissionerParams.deviceAddressUpdateDelegate = _pairingDelegateBridge; commissionerParams.pairingDelegate = _pairingDelegateBridge; @@ -482,11 +480,6 @@ - (BOOL)checkForInitError:(BOOL)condition logMsg:(NSString *)logMsg _persistentStorageDelegateBridge = NULL; } - if (_fabricStorage) { - delete _fabricStorage; - _fabricStorage = nullptr; - } - return YES; } @@ -503,6 +496,11 @@ - (BOOL)checkForStartError:(BOOL)condition logMsg:(NSString *)logMsg _cppCommissioner = NULL; } + if (_fabricStorage) { + delete _fabricStorage; + _fabricStorage = nullptr; + } + return YES; } @@ -520,4 +518,12 @@ - (BOOL)checkForError:(CHIP_ERROR)errorCode logMsg:(NSString *)logMsg error:(NSE return YES; } +- (void)dealloc +{ + if (_fabricStorage) { + delete _fabricStorage; + _fabricStorage = nullptr; + } +} + @end From 102224c7acdf0b2f1eacba4bb720a1d1f130fb37 Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Mon, 25 Oct 2021 10:29:08 -0500 Subject: [PATCH 06/10] Stop checking for invalid uncompressed fabric Id --- src/transport/FabricTable.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/transport/FabricTable.cpp b/src/transport/FabricTable.cpp index 74bbe230469d20..8cbdac9938d49e 100644 --- a/src/transport/FabricTable.cpp +++ b/src/transport/FabricTable.cpp @@ -384,11 +384,6 @@ FabricInfo * FabricTable::FindFabricWithIndex(FabricIndex fabricIndex) FabricInfo * FabricTable::FindFabricWithCompressedId(CompressedFabricId fabricId) { - if (fabricId == kUndefinedCompressedFabricId) - { - return nullptr; - } - static_assert(kMaxValidFabricIndex <= UINT8_MAX, "Cannot create more fabrics than UINT8_MAX"); for (FabricIndex i = kMinValidFabricIndex; i <= kMaxValidFabricIndex; i++) { From 40ec03d7cf07119ca314a3c8e68f11404bf14943 Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Tue, 26 Oct 2021 14:32:01 -0700 Subject: [PATCH 07/10] Update naming inside FabricTable --- src/transport/FabricTable.cpp | 24 ++++++++++++------------ src/transport/FabricTable.h | 6 +++--- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/transport/FabricTable.cpp b/src/transport/FabricTable.cpp index 8cbdac9938d49e..18d4d9da660bec 100644 --- a/src/transport/FabricTable.cpp +++ b/src/transport/FabricTable.cpp @@ -39,7 +39,7 @@ CHIP_ERROR FabricInfo::SetFabricLabel(const CharSpan & fabricLabel) return CHIP_NO_ERROR; } -CHIP_ERROR FabricInfo::StoreIntoKVS(FabricStorage * kvs) +CHIP_ERROR FabricInfo::CommitToStorage(FabricStorage * storage) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -103,7 +103,7 @@ CHIP_ERROR FabricInfo::StoreIntoKVS(FabricStorage * kvs) memcpy(info->mNOCCert, mNOCCert.data(), mNOCCert.size()); } - err = kvs->SyncStore(mFabric, key, info, sizeof(StorableFabricInfo)); + err = storage->SyncStore(mFabric, key, info, sizeof(StorableFabricInfo)); if (err != CHIP_NO_ERROR) { ChipLogError(Discovery, "Error occurred calling SyncSetKeyValue: %s", chip::ErrorStr(err)); @@ -117,7 +117,7 @@ CHIP_ERROR FabricInfo::StoreIntoKVS(FabricStorage * kvs) return err; } -CHIP_ERROR FabricInfo::FetchFromKVS(FabricStorage * kvs) +CHIP_ERROR FabricInfo::LoadFromStorage(FabricStorage * storage) { CHIP_ERROR err = CHIP_NO_ERROR; char key[kKeySize]; @@ -134,7 +134,7 @@ CHIP_ERROR FabricInfo::FetchFromKVS(FabricStorage * kvs) NodeId nodeId; - SuccessOrExit(err = kvs->SyncLoad(mFabric, key, info, infoSize)); + SuccessOrExit(err = storage->SyncLoad(mFabric, key, info, infoSize)); mFabricId = Encoding::LittleEndian::HostSwap64(info->mFabricId); nodeId = Encoding::LittleEndian::HostSwap64(info->mNodeId); @@ -162,7 +162,7 @@ CHIP_ERROR FabricInfo::FetchFromKVS(FabricStorage * kvs) VerifyOrExit(mOperationalKey != nullptr, err = CHIP_ERROR_NO_MEMORY); SuccessOrExit(err = mOperationalKey->Deserialize(info->mOperationalKey)); - ChipLogProgress(Inet, "Loading certs from KVS"); + ChipLogProgress(Inet, "Loading certs from storage"); SuccessOrExit(err = SetRootCert(ByteSpan(info->mRootCert, rootCertLen))); // The compressed fabric ID doesn't change for a fabric over time. @@ -197,17 +197,17 @@ CHIP_ERROR FabricInfo::GetCompressedId(FabricId fabricId, NodeId nodeId, PeerId return CHIP_NO_ERROR; } -CHIP_ERROR FabricInfo::DeleteFromKVS(FabricStorage * kvs, FabricIndex id) +CHIP_ERROR FabricInfo::DeleteFromStorage(FabricStorage * storage, FabricIndex fabricIndex) { CHIP_ERROR err = CHIP_NO_ERROR; char key[kKeySize]; - ReturnErrorOnFailure(GenerateKey(id, key, sizeof(key))); + ReturnErrorOnFailure(GenerateKey(fabricIndex, key, sizeof(key))); - err = kvs->SyncDelete(id, key); + err = storage->SyncDelete(fabricIndex, key); if (err != CHIP_NO_ERROR) { - ChipLogDetail(Discovery, "Fabric %d is not yet configured", id); + ChipLogDetail(Discovery, "Fabric %d is not yet configured", ifabricIndexd); } return err; } @@ -424,7 +424,7 @@ CHIP_ERROR FabricTable::Store(FabricIndex id) fabric = FindFabricWithIndex(id); VerifyOrExit(fabric != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT); - err = fabric->StoreIntoKVS(mStorage); + err = fabric->CommitToStorage(mStorage); exit: if (err == CHIP_NO_ERROR && mDelegate != nullptr) { @@ -440,7 +440,7 @@ CHIP_ERROR FabricTable::LoadFromStorage(FabricInfo * fabric) if (!fabric->IsInitialized()) { - ReturnErrorOnFailure(fabric->FetchFromKVS(mStorage)); + ReturnErrorOnFailure(fabric->LoadFromStorage(mStorage)); } if (mDelegate != nullptr) @@ -538,7 +538,7 @@ CHIP_ERROR FabricTable::Delete(FabricIndex id) fabric = FindFabricWithIndex(id); fabricIsInitialized = fabric != nullptr && fabric->IsInitialized(); - err = FabricInfo::DeleteFromKVS(mStorage, id); // Delete from storage regardless + err = FabricInfo::DeleteFromStorage(mStorage, id); // Delete from storage regardless exit: if (err == CHIP_NO_ERROR) diff --git a/src/transport/FabricTable.h b/src/transport/FabricTable.h index 919cc3fefa77bb..584a7fa664bfa4 100644 --- a/src/transport/FabricTable.h +++ b/src/transport/FabricTable.h @@ -271,9 +271,9 @@ class DLL_EXPORT FabricInfo static CHIP_ERROR GenerateKey(FabricIndex id, char * key, size_t len); - CHIP_ERROR StoreIntoKVS(FabricStorage * kvs); - CHIP_ERROR FetchFromKVS(FabricStorage * kvs); - static CHIP_ERROR DeleteFromKVS(FabricStorage * kvs, FabricIndex id); + CHIP_ERROR CommitToStorage(FabricStorage * storage); + CHIP_ERROR LoadFromStorage(FabricStorage * storage); + static CHIP_ERROR DeleteFromStorage(FabricStorage * storage, FabricIndex fabricIndex); void ReleaseCert(MutableByteSpan & cert); void ReleaseOperationalCerts() From 43c42c3667787cc3800415f494b652898b17e8fd Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Tue, 26 Oct 2021 15:22:11 -0700 Subject: [PATCH 08/10] Update SimpleFabricStorage to preserve FabricIndex --- src/transport/FabricTable.cpp | 11 ++++++++++- src/transport/FabricTable.h | 17 ++++++++++++++--- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/transport/FabricTable.cpp b/src/transport/FabricTable.cpp index 18d4d9da660bec..2c6c98d47f3b72 100644 --- a/src/transport/FabricTable.cpp +++ b/src/transport/FabricTable.cpp @@ -207,7 +207,7 @@ CHIP_ERROR FabricInfo::DeleteFromStorage(FabricStorage * storage, FabricIndex fa err = storage->SyncDelete(fabricIndex, key); if (err != CHIP_NO_ERROR) { - ChipLogDetail(Discovery, "Fabric %d is not yet configured", ifabricIndexd); + ChipLogDetail(Discovery, "Fabric %d is not yet configured", fabricIndex); } return err; } @@ -600,4 +600,13 @@ CHIP_ERROR FabricTable::SetFabricDelegate(FabricTableDelegate * delegate) return CHIP_NO_ERROR; } +std::string SimpleFabricStorage::formatKey(FabricIndex fabricIndex, const char * key) +{ + char fabricPrefix[fabricPrefixSize]; + snprintf(fabricPrefix, fabricPrefixSize, "F%02X/", fabricIndex); + std::string formattedKey = fabricPrefix; + formattedKey += key; + return formattedKey; +} + } // namespace chip diff --git a/src/transport/FabricTable.h b/src/transport/FabricTable.h index 584a7fa664bfa4..5b5b7244c43c16 100644 --- a/src/transport/FabricTable.h +++ b/src/transport/FabricTable.h @@ -73,6 +73,9 @@ class DLL_EXPORT FabricStorage * @brief A default implementation of Fabric storage that preserves legacy behavior of using * the Persistent storage delegate directly. * + * This class automatically prefixes the Fabric Storage Keys with the FabricIndex. + * The keys are formatted like so: "F%02X/" + key. + * */ class DLL_EXPORT SimpleFabricStorage : public FabricStorage { @@ -91,22 +94,30 @@ class DLL_EXPORT SimpleFabricStorage : public FabricStorage CHIP_ERROR SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override { VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); - return mStorage->SyncSetKeyValue(key, buffer, size); + std::string formattedKey = formatKey(fabricIndex, key); + return mStorage->SyncSetKeyValue(formattedKey.c_str(), buffer, size); }; CHIP_ERROR SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override { VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); - return mStorage->SyncGetKeyValue(key, buffer, size); + std::string formattedKey = formatKey(fabricIndex, key); + return mStorage->SyncGetKeyValue(formattedKey.c_str(), buffer, size); }; CHIP_ERROR SyncDelete(FabricIndex fabricIndex, const char * key) override { VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); - return mStorage->SyncDeleteKeyValue(key); + std::string formattedKey = formatKey(fabricIndex, key); + return mStorage->SyncDeleteKeyValue(formattedKey.c_str()); }; private: + // FabricPrefix is of the format "F%02X/" + const static int fabricPrefixSize = 5; + // Returns a string that adds a FabricIndex prefix to the Key + std::string formatKey(FabricIndex fabricIndex, const char * key); + PersistentStorageDelegate * mStorage = nullptr; }; From 8062a9b14d31e1e683c27fe0a8b8fffc989565c1 Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Wed, 27 Oct 2021 12:26:23 -0700 Subject: [PATCH 09/10] Remove usage of std::string inside SimpleFabricStorage --- src/transport/FabricTable.cpp | 43 ++++++++++++++++++++++++++++++----- src/transport/FabricTable.h | 26 ++++----------------- 2 files changed, 41 insertions(+), 28 deletions(-) diff --git a/src/transport/FabricTable.cpp b/src/transport/FabricTable.cpp index 2c6c98d47f3b72..8c764319093579 100644 --- a/src/transport/FabricTable.cpp +++ b/src/transport/FabricTable.cpp @@ -600,13 +600,44 @@ CHIP_ERROR FabricTable::SetFabricDelegate(FabricTableDelegate * delegate) return CHIP_NO_ERROR; } -std::string SimpleFabricStorage::formatKey(FabricIndex fabricIndex, const char * key) +CHIP_ERROR formatKey(FabricIndex fabricIndex, MutableCharSpan & formattedKey, const char * key) { - char fabricPrefix[fabricPrefixSize]; - snprintf(fabricPrefix, fabricPrefixSize, "F%02X/", fabricIndex); - std::string formattedKey = fabricPrefix; - formattedKey += key; - return formattedKey; + CHIP_ERROR err = CHIP_NO_ERROR; + + int res = snprintf(formattedKey.data(), formattedKey.size(), "F%02X/%s", fabricIndex, key); + if (res < 0 || (size_t) res >= formattedKey.size()) + { + ChipLogError(Discovery, "Failed to format Key %s. snprintf error: %d", key, res); + return CHIP_ERROR_NO_MEMORY; + } + return err; } +CHIP_ERROR SimpleFabricStorage::SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) +{ + VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); + char formattedKey[MAX_KEY_SIZE] = ""; + MutableCharSpan keySpan = MutableCharSpan(formattedKey, MAX_KEY_SIZE); + ReturnErrorOnFailure(formatKey(fabricIndex, keySpan, key)); + return mStorage->SyncSetKeyValue(formattedKey, buffer, size); +}; + +CHIP_ERROR SimpleFabricStorage::SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) +{ + VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); + char formattedKey[MAX_KEY_SIZE] = ""; + MutableCharSpan keySpan = MutableCharSpan(formattedKey, MAX_KEY_SIZE); + ReturnErrorOnFailure(formatKey(fabricIndex, keySpan, key)); + return mStorage->SyncGetKeyValue(formattedKey, buffer, size); +}; + +CHIP_ERROR SimpleFabricStorage::SyncDelete(FabricIndex fabricIndex, const char * key) +{ + VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); + char formattedKey[MAX_KEY_SIZE] = ""; + MutableCharSpan keySpan = MutableCharSpan(formattedKey, MAX_KEY_SIZE); + ReturnErrorOnFailure(formatKey(fabricIndex, keySpan, key)); + return mStorage->SyncDeleteKeyValue(formattedKey); +}; + } // namespace chip diff --git a/src/transport/FabricTable.h b/src/transport/FabricTable.h index 5b5b7244c43c16..d4ef3ef66a738a 100644 --- a/src/transport/FabricTable.h +++ b/src/transport/FabricTable.h @@ -91,32 +91,14 @@ class DLL_EXPORT SimpleFabricStorage : public FabricStorage return CHIP_NO_ERROR; } - CHIP_ERROR SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override - { - VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); - std::string formattedKey = formatKey(fabricIndex, key); - return mStorage->SyncSetKeyValue(formattedKey.c_str(), buffer, size); - }; + CHIP_ERROR SyncStore(FabricIndex fabricIndex, const char * key, const void * buffer, uint16_t size) override; - CHIP_ERROR SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override - { - VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); - std::string formattedKey = formatKey(fabricIndex, key); - return mStorage->SyncGetKeyValue(formattedKey.c_str(), buffer, size); - }; + CHIP_ERROR SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override; - CHIP_ERROR SyncDelete(FabricIndex fabricIndex, const char * key) override - { - VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); - std::string formattedKey = formatKey(fabricIndex, key); - return mStorage->SyncDeleteKeyValue(formattedKey.c_str()); - }; + CHIP_ERROR SyncDelete(FabricIndex fabricIndex, const char * key) override; private: - // FabricPrefix is of the format "F%02X/" - const static int fabricPrefixSize = 5; - // Returns a string that adds a FabricIndex prefix to the Key - std::string formatKey(FabricIndex fabricIndex, const char * key); + const static int MAX_KEY_SIZE = 32; PersistentStorageDelegate * mStorage = nullptr; }; From fdbd7ee826763497e1010d2f364653bb700009b0 Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Wed, 27 Oct 2021 12:31:41 -0700 Subject: [PATCH 10/10] Pass the mutable char spans by value instead --- src/transport/FabricTable.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/transport/FabricTable.cpp b/src/transport/FabricTable.cpp index 8c764319093579..ef1f9ed45293fc 100644 --- a/src/transport/FabricTable.cpp +++ b/src/transport/FabricTable.cpp @@ -600,7 +600,7 @@ CHIP_ERROR FabricTable::SetFabricDelegate(FabricTableDelegate * delegate) return CHIP_NO_ERROR; } -CHIP_ERROR formatKey(FabricIndex fabricIndex, MutableCharSpan & formattedKey, const char * key) +CHIP_ERROR formatKey(FabricIndex fabricIndex, MutableCharSpan formattedKey, const char * key) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -617,8 +617,7 @@ CHIP_ERROR SimpleFabricStorage::SyncStore(FabricIndex fabricIndex, const char * { VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); char formattedKey[MAX_KEY_SIZE] = ""; - MutableCharSpan keySpan = MutableCharSpan(formattedKey, MAX_KEY_SIZE); - ReturnErrorOnFailure(formatKey(fabricIndex, keySpan, key)); + ReturnErrorOnFailure(formatKey(fabricIndex, MutableCharSpan(formattedKey, MAX_KEY_SIZE), key)); return mStorage->SyncSetKeyValue(formattedKey, buffer, size); }; @@ -626,8 +625,7 @@ CHIP_ERROR SimpleFabricStorage::SyncLoad(FabricIndex fabricIndex, const char * k { VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); char formattedKey[MAX_KEY_SIZE] = ""; - MutableCharSpan keySpan = MutableCharSpan(formattedKey, MAX_KEY_SIZE); - ReturnErrorOnFailure(formatKey(fabricIndex, keySpan, key)); + ReturnErrorOnFailure(formatKey(fabricIndex, MutableCharSpan(formattedKey, MAX_KEY_SIZE), key)); return mStorage->SyncGetKeyValue(formattedKey, buffer, size); }; @@ -635,8 +633,7 @@ CHIP_ERROR SimpleFabricStorage::SyncDelete(FabricIndex fabricIndex, const char * { VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); char formattedKey[MAX_KEY_SIZE] = ""; - MutableCharSpan keySpan = MutableCharSpan(formattedKey, MAX_KEY_SIZE); - ReturnErrorOnFailure(formatKey(fabricIndex, keySpan, key)); + ReturnErrorOnFailure(formatKey(fabricIndex, MutableCharSpan(formattedKey, MAX_KEY_SIZE), key)); return mStorage->SyncDeleteKeyValue(formattedKey); };