Remove FabricStorage (#15811)

* Remove FabricStorage

* Fixed up a dangling uninitialized CHIP_ERROR in the Python C++ code

Co-authored-by: Jerry Johns <johnsj@google.com>

examples/chip-tool/commands/common/CHIPCommand.cpp

@@ -46,10 +46,8 @@ CHIP_ERROR CHIPCommand::Run()
 #endif
 
     ReturnLogErrorOnFailure(mDefaultStorage.Init());
-    ReturnLogErrorOnFailure(mFabricStorage.Initialize(&mDefaultStorage));
 
     chip::Controller::FactoryInitParams factoryInitParams;
-    factoryInitParams.fabricStorage            = &mFabricStorage;
     factoryInitParams.fabricIndependentStorage = &mDefaultStorage;
     factoryInitParams.listenPort               = static_cast<uint16_t>(mDefaultStorage.GetListenPort() + CurrentCommissionerId());
     ReturnLogErrorOnFailure(DeviceControllerFactory::GetInstance().Init(factoryInitParams));

examples/chip-tool/commands/common/CHIPCommand.h

@@ -91,7 +91,6 @@ class CHIPCommand : public Command
 
     PersistentStorage mDefaultStorage;
     PersistentStorage mCommissionerStorage;
-    chip::SimpleFabricStorage mFabricStorage;
     CredentialIssuerCommands * mCredIssuerCmds;
 
     std::string GetIdentity();

examples/platform/linux/AppMain.cpp

@@ -310,7 +310,6 @@ DeviceCommissioner gCommissioner;
 CommissionerDiscoveryController gCommissionerDiscoveryController;
 MyCommissionerCallback gCommissionerCallback;
 MyServerStorageDelegate gServerStorage;
-SimpleFabricStorage gFabricStorage;
 ExampleOperationalCredentialsIssuer gOpCredsIssuer;
 NodeId gLocalId = kMaxOperationalNodeId;
 
@@ -319,9 +318,6 @@ CHIP_ERROR InitCommissioner()
     Controller::FactoryInitParams factoryParams;
     Controller::SetupParams params;
 
-    ReturnErrorOnFailure(gFabricStorage.Initialize(&gServerStorage));
-
-    factoryParams.fabricStorage = &gFabricStorage;
     // use a different listen port for the commissioner than the default used by chip-tool.
     factoryParams.listenPort               = LinuxDeviceOptions::GetInstance().securedCommissionerPort + 10;
     factoryParams.fabricIndependentStorage = &gServerStorage;

src/app/server/Server.h

@@ -113,7 +113,7 @@ class Server
 
     static Server sServer;
 
-    class DeviceStorageDelegate : public PersistentStorageDelegate, public FabricStorage
+    class DeviceStorageDelegate : public PersistentStorageDelegate
     {
         CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override
         {
@@ -137,18 +137,6 @@ class Server
         {
             return DeviceLayer::PersistedStorage::KeyValueStoreMgr().Delete(key);
         }
-
-        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); };
     };
 
     class GroupDataProviderListener final : public Credentials::GroupDataProvider::GroupListener

src/controller/CHIPDeviceControllerFactory.cpp

@@ -54,7 +54,6 @@ CHIP_ERROR DeviceControllerFactory::Init(FactoryInitParams params)
     }
 
     mListenPort               = params.listenPort;
-    mFabricStorage            = params.fabricStorage;
     mFabricIndependentStorage = params.fabricIndependentStorage;
 
     CHIP_ERROR err = InitSystemState(params);
@@ -145,7 +144,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
     stateParams.exchangeMgr           = chip::Platform::New<Messaging::ExchangeManager>();
     stateParams.messageCounterManager = chip::Platform::New<secure_channel::MessageCounterManager>();
 
-    ReturnErrorOnFailure(stateParams.fabricTable->Init(mFabricStorage));
+    ReturnErrorOnFailure(stateParams.fabricTable->Init(params.fabricIndependentStorage));
 
     ReturnErrorOnFailure(stateParams.sessionMgr->Init(stateParams.systemLayer, stateParams.transportMgr,
                                                       stateParams.messageCounterManager, params.fabricIndependentStorage,
@@ -269,7 +268,6 @@ void DeviceControllerFactory::Shutdown()
         chip::Platform::Delete(mSystemState);
         mSystemState = nullptr;
     }
-    mFabricStorage            = nullptr;
     mFabricIndependentStorage = nullptr;
 }
 

src/controller/CHIPDeviceControllerFactory.h

@@ -74,7 +74,6 @@ struct SetupParams
 // We're blocked because of the need to support !CHIP_DEVICE_LAYER
 struct FactoryInitParams
 {
-    FabricStorage * fabricStorage                                 = nullptr;
     System::Layer * systemLayer                                   = nullptr;
     PersistentStorageDelegate * fabricIndependentStorage          = nullptr;
     Inet::EndPointManager<Inet::TCPEndPoint> * tcpEndPointManager = nullptr;
@@ -145,7 +144,6 @@ class DeviceControllerFactory
     CHIP_ERROR InitSystemState();
 
     uint16_t mListenPort;
-    FabricStorage * mFabricStorage                        = nullptr;
     DeviceControllerSystemState * mSystemState            = nullptr;
     PersistentStorageDelegate * mFabricIndependentStorage = nullptr;
 };

src/controller/java/AndroidDeviceControllerWrapper.cpp

@@ -118,7 +118,6 @@ AndroidDeviceControllerWrapper::AllocateNew(JavaVM * vm, jobject deviceControlle
     initParams.systemLayer        = systemLayer;
     initParams.tcpEndPointManager = tcpEndPointManager;
     initParams.udpEndPointManager = udpEndPointManager;
-    initParams.fabricStorage      = wrapper.get();
 
     // move bleLayer into platform/android to share with app server
 #if CONFIG_NETWORK_LAYER_BLE
@@ -329,19 +328,3 @@ 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(chip::FabricIndex fabricIndex, const char * key, const void * buffer,
-                                                     uint16_t size)
-{
-    return SyncSetKeyValue(key, buffer, size);
-};
-
-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)
-{
-    return SyncDeleteKeyValue(key);
-};

src/controller/java/AndroidDeviceControllerWrapper.h

@@ -35,9 +35,7 @@
  *
  * Generally it contains the DeviceController class itself, plus any related delegates/callbacks.
  */
-class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDelegate,
-                                       public chip::PersistentStorageDelegate,
-                                       public chip::FabricStorage
+class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDelegate, public chip::PersistentStorageDelegate
 {
 public:
     ~AndroidDeviceControllerWrapper();
@@ -66,11 +64,6 @@ 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(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)
     {
         return reinterpret_cast<AndroidDeviceControllerWrapper *>(handle);

src/controller/python/ChipDeviceController-ScriptBinding.cpp

@@ -83,7 +83,6 @@ typedef void (*ChipThreadTaskRunnerFunct)(intptr_t context);
 }
 
 namespace {
-chip::SimpleFabricStorage sFabricStorage;
 chip::Platform::ScopedMemoryBuffer<uint8_t> sSsidBuf;
 chip::Platform::ScopedMemoryBuffer<uint8_t> sCredsBuf;
 chip::Platform::ScopedMemoryBuffer<uint8_t> sThreadBuf;
@@ -211,15 +210,9 @@ chip::Controller::Python::StorageAdapter * pychip_Storage_GetStorageAdapter()
 
 ChipError::StorageType pychip_DeviceController_StackInit()
 {
-    CHIP_ERROR err;
-
     VerifyOrDie(sStorageAdapter != nullptr);
 
-    err = sFabricStorage.Initialize(sStorageAdapter);
-    VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger());
-
     FactoryInitParams factoryParams;
-    factoryParams.fabricStorage            = &sFabricStorage;
     factoryParams.fabricIndependentStorage = sStorageAdapter;
     factoryParams.enableServerInteractions = true;
 

src/controller/python/chip/internal/CommissionerImpl.cpp

@@ -82,7 +82,6 @@ class ScriptDevicePairingDelegate final : public chip::Controller::DevicePairing
 };
 
 ServerStorageDelegate gServerStorage;
-chip::SimpleFabricStorage gFabricStorage;
 ScriptDevicePairingDelegate gPairingDelegate;
 chip::Controller::ExampleOperationalCredentialsIssuer gOperationalCredentialsIssuer;
 
@@ -116,10 +115,6 @@ extern "C" chip::Controller::DeviceCommissioner * pychip_internal_Commissioner_N
         const chip::Credentials::AttestationTrustStore * testingRootStore = chip::Credentials::GetTestAttestationTrustStore();
         chip::Credentials::SetDeviceAttestationVerifier(chip::Credentials::GetDefaultDACVerifier(testingRootStore));
 
-        err = gFabricStorage.Initialize(&gServerStorage);
-        SuccessOrExit(err);
-
-        factoryParams.fabricStorage            = &gFabricStorage;
         factoryParams.fabricIndependentStorage = &gServerStorage;
 
         commissionerParams.pairingDelegate = &gPairingDelegate;

src/credentials/FabricTable.cpp

@@ -41,7 +41,7 @@ CHIP_ERROR FabricInfo::SetFabricLabel(const CharSpan & fabricLabel)
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR FabricInfo::CommitToStorage(FabricStorage * storage)
+CHIP_ERROR FabricInfo::CommitToStorage(PersistentStorageDelegate * storage)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
 
@@ -102,7 +102,7 @@ CHIP_ERROR FabricInfo::CommitToStorage(FabricStorage * storage)
         memcpy(info->mNOCCert, mNOCCert.data(), mNOCCert.size());
     }
 
-    err = storage->SyncStore(mFabric, key, info, sizeof(StorableFabricInfo));
+    err = storage->SyncSetKeyValue(key, info, sizeof(StorableFabricInfo));
     if (err != CHIP_NO_ERROR)
     {
         ChipLogError(Discovery, "Error occurred calling SyncSetKeyValue: %s", chip::ErrorStr(err));
@@ -116,7 +116,7 @@ CHIP_ERROR FabricInfo::CommitToStorage(FabricStorage * storage)
     return err;
 }
 
-CHIP_ERROR FabricInfo::LoadFromStorage(FabricStorage * storage)
+CHIP_ERROR FabricInfo::LoadFromStorage(PersistentStorageDelegate * storage)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
     char key[kKeySize];
@@ -132,7 +132,7 @@ CHIP_ERROR FabricInfo::LoadFromStorage(FabricStorage * storage)
     size_t stringLength;
     NodeId nodeId;
 
-    SuccessOrExit(err = storage->SyncLoad(mFabric, key, info, infoSize));
+    SuccessOrExit(err = storage->SyncGetKeyValue(key, info, infoSize));
 
     id          = info->mFabricIndex;
     mVendorId   = Encoding::LittleEndian::HostSwap16(info->mVendorId);
@@ -210,14 +210,14 @@ CHIP_ERROR FabricInfo::GeneratePeerId(FabricId fabricId, NodeId nodeId, PeerId *
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR FabricInfo::DeleteFromStorage(FabricStorage * storage, FabricIndex fabricIndex)
+CHIP_ERROR FabricInfo::DeleteFromStorage(PersistentStorageDelegate * storage, FabricIndex fabricIndex)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
 
     char key[kKeySize];
     ReturnErrorOnFailure(GenerateKey(fabricIndex, key, sizeof(key)));
 
-    err = storage->SyncDelete(fabricIndex, key);
+    err = storage->SyncDeleteKeyValue(key);
     if (err != CHIP_NO_ERROR)
     {
         ChipLogDetail(Discovery, "Fabric %d is not yet configured", fabricIndex);
@@ -648,7 +648,7 @@ void FabricTable::DeleteAllFabrics()
     }
 }
 
-CHIP_ERROR FabricTable::Init(FabricStorage * storage)
+CHIP_ERROR FabricTable::Init(PersistentStorageDelegate * storage)
 {
     VerifyOrReturnError(storage != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
     mStorage = storage;
@@ -699,28 +699,4 @@ CHIP_ERROR formatKey(FabricIndex fabricIndex, MutableCharSpan formattedKey, cons
     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] = "";
-    ReturnErrorOnFailure(formatKey(fabricIndex, MutableCharSpan(formattedKey, MAX_KEY_SIZE), 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] = "";
-    ReturnErrorOnFailure(formatKey(fabricIndex, MutableCharSpan(formattedKey, MAX_KEY_SIZE), 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] = "";
-    ReturnErrorOnFailure(formatKey(fabricIndex, MutableCharSpan(formattedKey, MAX_KEY_SIZE), key));
-    return mStorage->SyncDeleteKeyValue(formattedKey);
-};
-
 } // namespace chip

src/credentials/FabricTable.h

@@ -54,61 +54,6 @@ static_assert(kUndefinedFabricIndex < chip::kMinValidFabricIndex, "Undefined fab
 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.
- *
- *        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
-{
-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;
-
-    CHIP_ERROR SyncLoad(FabricIndex fabricIndex, const char * key, void * buffer, uint16_t & size) override;
-
-    CHIP_ERROR SyncDelete(FabricIndex fabricIndex, const char * key) override;
-
-private:
-    const static int MAX_KEY_SIZE = 32;
-
-    PersistentStorageDelegate * mStorage = nullptr;
-};
-
 /**
  * Defines state of a pairing established by a fabric.
  * Node ID is only settable using the device operational credentials.
@@ -290,9 +235,9 @@ class DLL_EXPORT FabricInfo
 
     static CHIP_ERROR GenerateKey(FabricIndex id, char * key, size_t len);
 
-    CHIP_ERROR CommitToStorage(FabricStorage * storage);
-    CHIP_ERROR LoadFromStorage(FabricStorage * storage);
-    static CHIP_ERROR DeleteFromStorage(FabricStorage * storage, FabricIndex fabricIndex);
+    CHIP_ERROR CommitToStorage(PersistentStorageDelegate * storage);
+    CHIP_ERROR LoadFromStorage(PersistentStorageDelegate * storage);
+    static CHIP_ERROR DeleteFromStorage(PersistentStorageDelegate * storage, FabricIndex fabricIndex);
 
     void ReleaseCert(MutableByteSpan & cert);
     void ReleaseOperationalCerts()
@@ -449,7 +394,7 @@ class DLL_EXPORT FabricTable
 
     void Reset();
 
-    CHIP_ERROR Init(FabricStorage * storage);
+    CHIP_ERROR Init(PersistentStorageDelegate * storage);
     CHIP_ERROR AddFabricDelegate(FabricTableDelegate * delegate);
 
     uint8_t FabricCount() const { return mFabricCount; }
@@ -461,7 +406,7 @@ class DLL_EXPORT FabricTable
 
 private:
     FabricInfo mStates[CHIP_CONFIG_MAX_FABRICS];
-    FabricStorage * mStorage = nullptr;
+    PersistentStorageDelegate * mStorage = nullptr;
 
     FabricTableDelegate * mDelegate = nullptr;