From 97b144aca3ff606a8bc3db47c7ad775eab7f51d3 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 11 Apr 2022 18:58:33 -0400 Subject: [PATCH] Remove unused per-controller storage delegate (#17258) * Remove unused DoesDevicePairingExist function. * Remove write-only mPairedDevicesUpdated field * Remove write-only mPairedDevices from DeviceController. The comment about initializing the commissionee device pool was out of date; the code being removed does not touch that pool. * Remove use of no-longer-existing storage keys from disabled chip-tool code. * Remove unused storage delegate from ControllerDeviceInitParams. * Remove write-only storage delegate field from ControllerInitParams. * Remove write-only storage delegate field from Controller::SetupParams. --- .../chip-tool/commands/common/CHIPCommand.cpp | 1 - .../pairing/CommissionedListCommand.cpp | 15 ++-- examples/platform/linux/AppMain.cpp | 1 - src/controller/CHIPDeviceController.cpp | 84 +------------------ src/controller/CHIPDeviceController.h | 19 ----- .../CHIPDeviceControllerFactory.cpp | 1 - src/controller/CHIPDeviceControllerFactory.h | 2 - src/controller/CommissioneeDeviceProxy.h | 5 +- .../java/AndroidDeviceControllerWrapper.cpp | 5 +- src/controller/python/OpCredsBinding.cpp | 2 - .../python/chip/internal/CommissionerImpl.cpp | 1 - .../Framework/CHIP/CHIPDeviceController.mm | 1 - src/lib/support/PersistentStorageMacros.h | 3 - 13 files changed, 9 insertions(+), 131 deletions(-) diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp index 6a9a1f74615c08..f7924101520a50 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.cpp +++ b/examples/chip-tool/commands/common/CHIPCommand.cpp @@ -334,7 +334,6 @@ CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricId f commissionerParams.controllerNOC = nocSpan; } - commissionerParams.storageDelegate = &mCommissionerStorage; // TODO: Initialize IPK epoch key in ExampleOperationalCredentials issuer rather than relying on DefaultIpkValue commissionerParams.operationalCredentialsDelegate = mCredIssuerCmds->GetCredentialIssuer(); commissionerParams.controllerVendorId = chip::VendorId::TestVendor1; diff --git a/examples/chip-tool/commands/pairing/CommissionedListCommand.cpp b/examples/chip-tool/commands/pairing/CommissionedListCommand.cpp index ed617fc07cf6d9..ef30ef4978819f 100644 --- a/examples/chip-tool/commands/pairing/CommissionedListCommand.cpp +++ b/examples/chip-tool/commands/pairing/CommissionedListCommand.cpp @@ -35,12 +35,8 @@ CHIP_ERROR CommissionedListCommand::PrintInformation() uint16_t pairedNodesIdsSize = sizeof(pairedNodesIds); memset(pairedNodesIds, 0, pairedNodesIdsSize); - PERSISTENT_KEY_OP(static_cast(0), chip::kPairedDeviceListKeyPrefix, key, - ReturnLogErrorOnFailure(mStorage.SyncGetKeyValue(key, pairedNodesIds, pairedNodesIdsSize))); - - chip::SerializableU64Set devices; - devices.Deserialize(chip::ByteSpan((uint8_t *) pairedNodesIds, pairedNodesIdsSize)); - + // TODO: Get the list of paired node IDs. chip-tool needs to store that as + // devices get paired. uint16_t pairedDevicesCount = 0; while (pairedNodesIds[pairedDevicesCount] != 0x0 && pairedDevicesCount < chip::Controller::kNumMaxPairedDevices) { @@ -69,13 +65,12 @@ CHIP_ERROR CommissionedListCommand::PrintInformation() CHIP_ERROR CommissionedListCommand::PrintDeviceInformation(chip::NodeId deviceId) { + // TODO: Controller::SerializedDevice and Controller::SerializableDevice are + // gone. Need to figure out what chip-tool should actually store/retrieve + // here. chip::Controller::SerializedDevice deviceInfo; uint16_t size = sizeof(deviceInfo.inner); - PERSISTENT_KEY_OP(deviceId, chip::kPairedDeviceKeyPrefix, key, - ReturnLogErrorOnFailure(mStorage.SyncGetKeyValue(key, deviceInfo.inner, size))); - VerifyOrReturnError(size <= sizeof(deviceInfo.inner), CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR); - chip::Controller::SerializableDevice serializable; constexpr size_t maxlen = BASE64_ENCODED_LEN(sizeof(serializable)); const size_t len = strnlen(chip::Uint8::to_const_char(&deviceInfo.inner[0]), maxlen); diff --git a/examples/platform/linux/AppMain.cpp b/examples/platform/linux/AppMain.cpp index 0c3f2b70d27042..b4d91babf35fb1 100644 --- a/examples/platform/linux/AppMain.cpp +++ b/examples/platform/linux/AppMain.cpp @@ -456,7 +456,6 @@ CHIP_ERROR InitCommissioner() ReturnErrorOnFailure(gGroupDataProvider.Init()); factoryParams.groupDataProvider = &gGroupDataProvider; - params.storageDelegate = &gServerStorage; params.operationalCredentialsDelegate = &gOpCredsIssuer; ReturnErrorOnFailure(gOpCredsIssuer.Initialize(gServerStorage)); diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index b5115052ee2346..fc4ed02c5630be 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -105,9 +105,7 @@ constexpr uint32_t kSessionEstablishmentTimeout = 40 * kMillisecondsPerSecond; DeviceController::DeviceController() { - mState = State::NotInitialized; - mStorageDelegate = nullptr; - mPairedDevicesInitialized = false; + mState = State::NotInitialized; } CHIP_ERROR DeviceController::Init(ControllerInitParams params) @@ -118,7 +116,6 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params) VerifyOrReturnError(params.systemState->SystemLayer() != nullptr, CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError(params.systemState->UDPEndPointManager() != nullptr, CHIP_ERROR_INVALID_ARGUMENT); - mStorageDelegate = params.storageDelegate; #if CONFIG_NETWORK_LAYER_BLE VerifyOrReturnError(params.systemState->BleLayer() != nullptr, CHIP_ERROR_INVALID_ARGUMENT); #endif @@ -231,8 +228,6 @@ CHIP_ERROR DeviceController::Shutdown() mSystemState->SessionMgr()->ExpireAllPairingsForFabric(mFabricInfo->GetFabricIndex()); } - mStorageDelegate = nullptr; - if (mFabricInfo != nullptr) { mFabricInfo->Reset(); @@ -246,16 +241,6 @@ CHIP_ERROR DeviceController::Shutdown() return CHIP_NO_ERROR; } -bool DeviceController::DoesDevicePairingExist(const PeerId & deviceId) -{ - if (InitializePairedDeviceList() == CHIP_NO_ERROR) - { - return mPairedDevices.Contains(deviceId.GetNodeId()); - } - - return false; -} - void DeviceController::ReleaseOperationalDevice(NodeId remoteDeviceId) { VerifyOrReturn(mState == State::Initialized && mFabricInfo != nullptr, @@ -329,64 +314,6 @@ void DeviceController::OnFirstMessageDeliveryFailed(const SessionHandle & sessio } } -CHIP_ERROR DeviceController::InitializePairedDeviceList() -{ - CHIP_ERROR err = CHIP_NO_ERROR; - uint8_t * buffer = nullptr; - - VerifyOrExit(mStorageDelegate != nullptr, err = CHIP_ERROR_INCORRECT_STATE); - - if (!mPairedDevicesInitialized) - { - constexpr uint16_t max_size = sizeof(uint64_t) * kNumMaxPairedDevices; - buffer = static_cast(chip::Platform::MemoryCalloc(max_size, 1)); - uint16_t size = max_size; - - VerifyOrExit(buffer != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT); - - CHIP_ERROR lookupError = CHIP_NO_ERROR; - PERSISTENT_KEY_OP(static_cast(0), kPairedDeviceListKeyPrefix, key, - lookupError = mStorageDelegate->SyncGetKeyValue(key, buffer, size)); - - // It's ok to not have an entry for the Paired Device list. We treat it the same as having an empty list. - if (lookupError != CHIP_ERROR_KEY_NOT_FOUND) - { - VerifyOrExit(size <= max_size, err = CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR); - err = SetPairedDeviceList(ByteSpan(buffer, size)); - SuccessOrExit(err); - } - } - -exit: - if (buffer != nullptr) - { - chip::Platform::MemoryFree(buffer); - } - if (err != CHIP_NO_ERROR) - { - ChipLogError(Controller, "Failed to initialize the device list with error: %" CHIP_ERROR_FORMAT, err.Format()); - } - - return err; -} - -CHIP_ERROR DeviceController::SetPairedDeviceList(ByteSpan serialized) -{ - CHIP_ERROR err = mPairedDevices.Deserialize(serialized); - - if (err != CHIP_NO_ERROR) - { - ChipLogError(Controller, "Failed to recreate the device list with buffer %.*s\n", static_cast(serialized.size()), - serialized.data()); - } - else - { - mPairedDevicesInitialized = true; - } - - return err; -} - CHIP_ERROR DeviceController::GetPeerAddressAndPort(PeerId peerId, Inet::IPAddress & addr, uint16_t & port) { VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE); @@ -412,7 +339,6 @@ ControllerDeviceInitParams DeviceController::GetControllerDeviceInitParams() .sessionManager = mSystemState->SessionMgr(), .exchangeMgr = mSystemState->ExchangeMgr(), .udpEndPointManager = mSystemState->UDPEndPointManager(), - .storageDelegate = mStorageDelegate, .fabricsTable = mSystemState->Fabrics(), }; } @@ -423,7 +349,6 @@ DeviceCommissioner::DeviceCommissioner() : mDeviceNOCChainCallback(OnDeviceNOCChainGeneration, this), mSetUpCodePairer(this) { mPairingDelegate = nullptr; - mPairedDevicesUpdated = false; mDeviceBeingCommissioned = nullptr; mDeviceInPASEEstablishment = nullptr; } @@ -632,10 +557,6 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re VerifyOrExit(mState == State::Initialized, err = CHIP_ERROR_INCORRECT_STATE); VerifyOrExit(mDeviceInPASEEstablishment == nullptr, err = CHIP_ERROR_INCORRECT_STATE); - // This will initialize the commissionee device pool if it has not already been initialized. - err = InitializePairedDeviceList(); - SuccessOrExit(err); - // TODO(#13940): We need to specify the peer address for BLE transport in bindings. if (params.GetPeerAddress().GetTransportType() == Transport::Type::kBle || params.GetPeerAddress().GetTransportType() == Transport::Type::kUndefined) @@ -1380,9 +1301,6 @@ CHIP_ERROR DeviceCommissioner::OnOperationalCredentialsProvisioningCompletion(De mSystemState->SystemLayer()->CancelTimer(OnSessionEstablishmentTimeoutCallback, this); - mPairedDevices.Insert(device->GetDeviceId()); - mPairedDevicesUpdated = true; - if (mPairingDelegate != nullptr) { mPairingDelegate->OnStatusUpdate(DevicePairingDelegate::SecurePairingSuccess); diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 306dc050998ae2..5b5108e1feab4b 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -81,14 +81,12 @@ namespace Controller { using namespace chip::Protocols::UserDirectedCommissioning; constexpr uint16_t kNumMaxActiveDevices = CHIP_CONFIG_CONTROLLER_MAX_ACTIVE_DEVICES; -constexpr uint16_t kNumMaxPairedDevices = 128; // Raw functions for cluster callbacks void OnBasicFailure(void * context, CHIP_ERROR err); struct ControllerInitParams { - PersistentStorageDelegate * storageDelegate = nullptr; DeviceControllerSystemState * systemState = nullptr; DeviceDiscoveryDelegate * deviceDiscoveryDelegate = nullptr; OperationalCredentialsDelegate * operationalCredentialsDelegate = nullptr; @@ -148,12 +146,6 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public Abstr CHIP_ERROR GetPeerAddressAndPort(PeerId peerId, Inet::IPAddress & addr, uint16_t & port); - /** - * This function returns true if the device corresponding to `deviceId` has previously been commissioned - * on the fabric. - */ - bool DoesDevicePairingExist(const PeerId & deviceId); - /** * This function finds the device corresponding to deviceId, and establishes a secure connection with it. * Once the connection is successfully establishes (or if it's already connected), it calls `onConnectedDevice` @@ -247,21 +239,15 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public Abstr State mState; - SerializableU64Set mPairedDevices; - bool mPairedDevicesInitialized; - PeerId mLocalId = PeerId(); FabricId mFabricId = kUndefinedFabricId; FabricInfo * mFabricInfo = nullptr; - PersistentStorageDelegate * mStorageDelegate = nullptr; // TODO(cecille): Make this configuarable. static constexpr int kMaxCommissionableNodes = 10; Dnssd::DiscoveredNodeData mCommissionableNodes[kMaxCommissionableNodes]; DeviceControllerSystemState * mSystemState = nullptr; - CHIP_ERROR InitializePairedDeviceList(); - CHIP_ERROR SetPairedDeviceList(ByteSpan pairedDeviceSerializedSet); ControllerDeviceInitParams GetControllerDeviceInitParams(); OperationalCredentialsDelegate * mOperationalCredentialsDelegate; @@ -575,11 +561,6 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, DeviceProxy * mDeviceBeingCommissioned = nullptr; CommissioneeDeviceProxy * mDeviceInPASEEstablishment = nullptr; - /* This field is true when device pairing information changes, e.g. a new device is paired, or - the pairing for a device is removed. The DeviceCommissioner uses this to decide when to - persist the device list */ - bool mPairedDevicesUpdated; - CommissioningStage mCommissioningStage = CommissioningStage::kSecurePairing; bool mRunCommissioningAfterConnection = false; diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 18e2c08ede8851..664e96e087b2e3 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -253,7 +253,6 @@ void DeviceControllerFactory::PopulateInitParams(ControllerInitParams & controll controllerParams.controllerNOC = params.controllerNOC; controllerParams.controllerICAC = params.controllerICAC; controllerParams.controllerRCAC = params.controllerRCAC; - controllerParams.storageDelegate = params.storageDelegate; controllerParams.systemState = mSystemState; controllerParams.controllerVendorId = params.controllerVendorId; diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index a4880b5340c491..d0ddfd0d314136 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -42,8 +42,6 @@ struct SetupParams { 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 * operationalKeypair = nullptr; diff --git a/src/controller/CommissioneeDeviceProxy.h b/src/controller/CommissioneeDeviceProxy.h index 0d03253a73d154..e33f958b0de864 100644 --- a/src/controller/CommissioneeDeviceProxy.h +++ b/src/controller/CommissioneeDeviceProxy.h @@ -67,7 +67,6 @@ struct ControllerDeviceInitParams SessionManager * sessionManager = nullptr; Messaging::ExchangeManager * exchangeMgr = nullptr; Inet::EndPointManager * udpEndPointManager = nullptr; - PersistentStorageDelegate * storageDelegate = nullptr; #if CONFIG_NETWORK_LAYER_BLE Ble::BleLayer * bleLayer = nullptr; #endif @@ -168,8 +167,6 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegat * Update data of the device. * * This function will set new IP address, port and MRP retransmission intervals of the device. - * Since the device settings might have been moved from RAM to the persistent storage, the function - * will load the device settings first, before making the changes. * * @param[in] addr Address of the device to be set. * @param[in] config MRP parameters @@ -190,7 +187,7 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegat * @brief * Called to indicate this proxy has been paired successfully. * - * This causes the secure session parameters to be loaded and stores the session details in the session manager. + * This stores the session details in the session manager. */ CHIP_ERROR SetConnected(); diff --git a/src/controller/java/AndroidDeviceControllerWrapper.cpp b/src/controller/java/AndroidDeviceControllerWrapper.cpp index 39212b181dd11f..d0fe68ae3f6130 100644 --- a/src/controller/java/AndroidDeviceControllerWrapper.cpp +++ b/src/controller/java/AndroidDeviceControllerWrapper.cpp @@ -125,12 +125,11 @@ AndroidDeviceControllerWrapper::AllocateNew(JavaVM * vm, jobject deviceControlle initParams.bleLayer = DeviceLayer::ConnectivityMgr().GetBleLayer(); #endif initParams.listenPort = CHIP_PORT + 1; - setupParams.storageDelegate = wrapper.get(); setupParams.pairingDelegate = wrapper.get(); setupParams.operationalCredentialsDelegate = opCredsIssuer; - initParams.fabricIndependentStorage = setupParams.storageDelegate; + initParams.fabricIndependentStorage = wrapper.get(); - wrapper->mGroupDataProvider.SetStorageDelegate(setupParams.storageDelegate); + wrapper->mGroupDataProvider.SetStorageDelegate(wrapper.get()); CHIP_ERROR err = wrapper->mGroupDataProvider.Init(); if (err != CHIP_NO_ERROR) diff --git a/src/controller/python/OpCredsBinding.cpp b/src/controller/python/OpCredsBinding.cpp index 9935372bd2960c..d180e6542ae237 100644 --- a/src/controller/python/OpCredsBinding.cpp +++ b/src/controller/python/OpCredsBinding.cpp @@ -98,7 +98,6 @@ class OperationalCredentialsAdapter : public OperationalCredentialsDelegate } // namespace chip extern chip::Controller::Python::StorageAdapter * pychip_Storage_GetStorageAdapter(); -extern chip::Controller::Python::StorageAdapter * sStorageAdapter; extern chip::Credentials::GroupDataProviderImpl sGroupDataProvider; extern chip::Controller::ScriptDevicePairingDelegate sPairingDelegate; @@ -362,7 +361,6 @@ ChipError::StorageType pychip_OpCreds_AllocateController(OpCredsContext * contex VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger()); Controller::SetupParams initParams; - initParams.storageDelegate = sStorageAdapter; initParams.pairingDelegate = &sPairingDelegate; initParams.operationalCredentialsDelegate = context->mAdapter.get(); initParams.operationalKeypair = &ephemeralKey; diff --git a/src/controller/python/chip/internal/CommissionerImpl.cpp b/src/controller/python/chip/internal/CommissionerImpl.cpp index 1c66349012cc21..f7e1022995aad7 100644 --- a/src/controller/python/chip/internal/CommissionerImpl.cpp +++ b/src/controller/python/chip/internal/CommissionerImpl.cpp @@ -137,7 +137,6 @@ extern "C" chip::Controller::DeviceCommissioner * pychip_internal_Commissioner_N factoryParams.groupDataProvider = &gGroupDataProvider; commissionerParams.pairingDelegate = &gPairingDelegate; - commissionerParams.storageDelegate = &gServerStorage; err = ephemeralKey.Initialize(); SuccessOrExit(err); diff --git a/src/darwin/Framework/CHIP/CHIPDeviceController.mm b/src/darwin/Framework/CHIP/CHIPDeviceController.mm index d801ae1c41da1b..420f2fa65fd588 100644 --- a/src/darwin/Framework/CHIP/CHIPDeviceController.mm +++ b/src/darwin/Framework/CHIP/CHIPDeviceController.mm @@ -265,7 +265,6 @@ - (BOOL)startup:(_Nullable id)storageDelegate params.groupDataProvider = _groupDataProvider; params.fabricIndependentStorage = _persistentStorageDelegateBridge; - commissionerParams.storageDelegate = _persistentStorageDelegateBridge; commissionerParams.pairingDelegate = _pairingDelegateBridge; commissionerParams.operationalCredentialsDelegate = _operationalCredentialsDelegate; diff --git a/src/lib/support/PersistentStorageMacros.h b/src/lib/support/PersistentStorageMacros.h index e86ece0e64e659..57038b5751b861 100644 --- a/src/lib/support/PersistentStorageMacros.h +++ b/src/lib/support/PersistentStorageMacros.h @@ -27,9 +27,6 @@ namespace chip { -constexpr const char kPairedDeviceListKeyPrefix[] = "ListPairedDevices"; -constexpr const char kPairedDeviceKeyPrefix[] = "PairedDevice"; - // This macro generates a key for storage using a node ID and a key prefix, and performs the given action // on that key. #define PERSISTENT_KEY_OP(node, keyPrefix, key, action) \