Address review comments

Remove DefaultInstance() and weak cluster init function and instead provide a
DefaultThreadNetworkDirectoryServer sub-class that's easy to instantiate with
the default storage implementation.

Roll back in-memory state on persistent storage failure and add tests for this.

Add documentation about ByteSpan lifetimes in OperationalDataset class.

Add comments to MTRDemuxingStorage.mm

Author: ksperling-apple

examples/network-manager-app/linux/main.cpp

@@ -19,8 +19,11 @@
 #include <app/clusters/thread-network-directory-server/thread-network-directory-server.h>
 #include <app/clusters/wifi-network-management-server/wifi-network-management-server.h>
 #include <lib/core/CHIPSafeCasts.h>
+#include <lib/support/CodeUtils.h>
 #include <lib/support/Span.h>
 
+#include <optional>
+
 using namespace chip;
 using namespace chip::app;
 using namespace chip::app::Clusters;
@@ -33,6 +36,13 @@ ByteSpan ByteSpanFromCharSpan(CharSpan span)
     return ByteSpan(Uint8::from_const_char(span.data()), span.size());
 }
 
+std::optional<DefaultThreadNetworkDirectoryServer> gThreadNetworkDirectoryServer;
+void emberAfThreadNetworkDirectoryClusterServerInitCallback(chip::EndpointId endpoint)
+{
+    VerifyOrDie(!gThreadNetworkDirectoryServer.has_value());
+    gThreadNetworkDirectoryServer.emplace(endpoint).Init();
+}
+
 int main(int argc, char * argv[])
 {
     if (ChipLinuxAppInit(argc, argv) != 0)

src/app/chip_data_model.gni

@@ -392,7 +392,6 @@ template("chip_data_model") {
         ]
       } else if (cluster == "thread-network-directory-server") {
         sources += [
-          "${_app_root}/clusters/${cluster}/${cluster}-instance.cpp",
           "${_app_root}/clusters/${cluster}/${cluster}.cpp",
           "${_app_root}/clusters/${cluster}/${cluster}.h",
           "${_app_root}/clusters/${cluster}/DefaultThreadNetworkDirectoryStorage.cpp",

src/app/clusters/thread-network-directory-server/DefaultThreadNetworkDirectoryStorage.cpp

@@ -118,11 +118,18 @@ CHIP_ERROR DefaultThreadNetworkDirectoryStorage::RemoveNetwork(const ExtendedPan
     VerifyOrReturnError(FindNetwork(exPanId, index), CHIP_ERROR_NOT_FOUND);
 
     // Move subsequent elements down to fill the deleted slot
-    for (mCount--; index < mCount; index++)
+    index_t subsequent = --mCount - index;
+    auto * element     = &mExtendedPanIds[index];
+    memmove(element, element + 1, subsequent * sizeof(*element));
+    CHIP_ERROR err = StoreIndex();
+    if (err != CHIP_NO_ERROR)
     {
-        mExtendedPanIds[index] = mExtendedPanIds[index + 1];
+        // Roll back the change to our in-memory state
+        memmove(element + 1, element, subsequent * sizeof(*element));
+        mExtendedPanIds[index] = exPanId;
+        mCount++;
+        return err;
     }
-    ReturnErrorOnFailure(StoreIndex());
 
     // Delete the dataset itself. Ignore errors since we successfully updated the index.
     StorageKeyName key = DefaultStorageKeyAllocator::ThreadNetworkDirectoryDataset(exPanId.AsNumber());

src/app/clusters/thread-network-directory-server/thread-network-directory-server-instance.cpp

@@ -20,7 +20,9 @@
 #include <app-common/zap-generated/cluster-objects.h>
 #include <app/AttributeAccessInterface.h>
 #include <app/CommandHandlerInterface.h>
+#include <app/clusters/thread-network-directory-server/DefaultThreadNetworkDirectoryStorage.h>
 #include <app/clusters/thread-network-directory-server/ThreadNetworkDirectoryStorage.h>
+#include <app/server/Server.h>
 #include <lib/core/CHIPError.h>
 
 #include <optional>
@@ -32,17 +34,6 @@ namespace Clusters {
 class ThreadNetworkDirectoryServer : private AttributeAccessInterface, private CommandHandlerInterface
 {
 public:
-    /*
-     * Returns a default instance of this server cluster. Only available if there is
-     * exactly one instance of this cluster defined in the application zap file.
-     * Must not be called before the data model has been initialized.
-     *
-     * To support more than one cluster instance, or to customize how the cluster server
-     * is instantiated, the application should override the callback function
-     * emberAfThreadNetworkDirectoryClusterServerInitCallback(chip::EndpointId).
-     */
-    static ThreadNetworkDirectoryServer & DefaultInstance();
-
     ThreadNetworkDirectoryServer(EndpointId endpoint, ThreadNetworkDirectoryStorage & storage);
     ~ThreadNetworkDirectoryServer();
 
@@ -74,6 +65,22 @@ class ThreadNetworkDirectoryServer : private AttributeAccessInterface, private C
     ThreadNetworkDirectoryStorage & mStorage;
 };
 
+/**
+ * A ThreadNetworkDirectoryServer using DefaultThreadNetworkDirectoryStorage.
+ */
+class DefaultThreadNetworkDirectoryServer final : public ThreadNetworkDirectoryServer
+{
+public:
+    DefaultThreadNetworkDirectoryServer(EndpointId endpoint,
+                                        PersistentStorageDelegate & storage = Server::GetInstance().GetPersistentStorage()) :
+        ThreadNetworkDirectoryServer(endpoint, mStorage),
+        mStorage(storage)
+    {}
+
+private:
+    DefaultThreadNetworkDirectoryStorage mStorage;
+};
+
 } // namespace Clusters
 } // namespace app
 } // namespace chip

src/app/clusters/thread-network-directory-server/thread-network-directory-server.h

@@ -17,6 +17,7 @@
 
 #include <app/clusters/thread-network-directory-server/DefaultThreadNetworkDirectoryStorage.h>
 #include <lib/core/CHIPPersistentStorageDelegate.h>
+#include <lib/support/DefaultStorageKeyAllocator.h>
 #include <lib/support/TestPersistentStorageDelegate.h>
 
 #include <cstdint>
@@ -79,7 +80,7 @@ TEST_F(TestDefaultThreadNetworkDirectoryStorage, TestAddRemoveNetworks)
     const uint8_t datasetA[]        = { 1 };
     const uint8_t updatedDatasetA[] = { 0x11, 0x11 };
     const ThreadNetworkDirectoryStorage::ExtendedPanId panB(UINT64_C(0xe475cafef00df00d));
-    uint8_t datasetB[] = { 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 };
+    const uint8_t datasetB[] = { 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 };
     uint8_t mutableBytes[ThreadNetworkDirectoryStorage::kMaxThreadDatasetLen];
 
     // Add Pan A only
@@ -207,4 +208,85 @@ TEST_F(TestDefaultThreadNetworkDirectoryStorage, TestStorageOverflow)
     EXPECT_EQ(storage.AddOrUpdateNetwork(ThreadNetworkDirectoryStorage::ExtendedPanId(0u), ByteSpan(dataset)), CHIP_NO_ERROR);
 }
 
+TEST_F(TestDefaultThreadNetworkDirectoryStorage, TestAddNetworkStorageFailure)
+{
+    const ThreadNetworkDirectoryStorage::ExtendedPanId panA(UINT64_C(0xd00fd00fdeadbeef));
+    const uint8_t datasetA[] = { 1 };
+    const ThreadNetworkDirectoryStorage::ExtendedPanId panB(UINT64_C(0xe475cafef00df00d));
+    uint8_t datasetB[] = { 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 };
+
+    EXPECT_EQ(storage.AddOrUpdateNetwork(panA, ByteSpan(datasetA)), CHIP_NO_ERROR);
+
+    // Make the storage delegate return CHIP_ERROR_PERSISTED_STORAGE_FAILED for writes.
+    // Attempting to add a network should fail and state should be unaffected.
+    persistentStorageDelegate.SetRejectWrites(true);
+    EXPECT_EQ(storage.AddOrUpdateNetwork(panB, ByteSpan(datasetB)), CHIP_ERROR_PERSISTED_STORAGE_FAILED);
+    EXPECT_TRUE(storage.ContainsNetwork(panA));
+    EXPECT_FALSE(storage.ContainsNetwork(panB));
+    {
+        auto * iterator = storage.IterateNetworkIds();
+        EXPECT_EQ(iterator->Count(), 1u);
+        iterator->Release();
+    }
+}
+
+TEST_F(TestDefaultThreadNetworkDirectoryStorage, TestAddNetworkStorageWriteIndexFailure)
+{
+    const ThreadNetworkDirectoryStorage::ExtendedPanId panA(UINT64_C(0xd00fd00fdeadbeef));
+    const uint8_t datasetA[] = { 1 };
+    const ThreadNetworkDirectoryStorage::ExtendedPanId panB(UINT64_C(0xe475cafef00df00d));
+    uint8_t datasetB[] = { 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 };
+
+    EXPECT_EQ(storage.AddOrUpdateNetwork(panA, ByteSpan(datasetA)), CHIP_NO_ERROR);
+
+    // White box test: Poison the index key specifically, so the dataset
+    // can be written but the subsequent update of the index fails.
+    // Attempting to add a network should fail and state should be unaffected.
+    StorageKeyName indexKey = DefaultStorageKeyAllocator::ThreadNetworkDirectoryIndex();
+    persistentStorageDelegate.AddPoisonKey(indexKey.KeyName());
+    EXPECT_EQ(storage.AddOrUpdateNetwork(panB, ByteSpan(datasetB)), CHIP_ERROR_PERSISTED_STORAGE_FAILED);
+    EXPECT_TRUE(storage.ContainsNetwork(panA));
+    EXPECT_FALSE(storage.ContainsNetwork(panB));
+    {
+        auto * iterator = storage.IterateNetworkIds();
+        EXPECT_EQ(iterator->Count(), 1u);
+        iterator->Release();
+    }
+}
+
+TEST_F(TestDefaultThreadNetworkDirectoryStorage, TestRemoveNetworkStorageFailure)
+{
+    const ThreadNetworkDirectoryStorage::ExtendedPanId panA(UINT64_C(0xd00f0001));
+    const ThreadNetworkDirectoryStorage::ExtendedPanId panB(UINT64_C(0xd00f0002));
+    const ThreadNetworkDirectoryStorage::ExtendedPanId panC(UINT64_C(0xd00f0003));
+    const uint8_t dataset[] = { 1 };
+
+    // Just in case the test is compiled with CHIP_CONFIG_MAX_FABRICS == 1
+    bool canStorePanC = (storage.Capacity() >= 3);
+
+    // Add Pans A, B, and C
+    EXPECT_EQ(storage.AddOrUpdateNetwork(panA, ByteSpan(dataset)), CHIP_NO_ERROR);
+    EXPECT_EQ(storage.AddOrUpdateNetwork(panB, ByteSpan(dataset)), CHIP_NO_ERROR);
+    if (canStorePanC)
+    {
+        EXPECT_EQ(storage.AddOrUpdateNetwork(panC, ByteSpan(dataset)), CHIP_NO_ERROR);
+    }
+
+    // Make the storage delegate return CHIP_ERROR_PERSISTED_STORAGE_FAILED for writes.
+    // Attempting to remove a network should fail and state should be unaffected.
+    persistentStorageDelegate.SetRejectWrites(true);
+    EXPECT_EQ(storage.RemoveNetwork(panA), CHIP_ERROR_PERSISTED_STORAGE_FAILED);
+    EXPECT_TRUE(storage.ContainsNetwork(panA));
+    EXPECT_TRUE(storage.ContainsNetwork(panB));
+    if (canStorePanC)
+    {
+        EXPECT_TRUE(storage.ContainsNetwork(panC));
+    }
+    {
+        auto * iterator = storage.IterateNetworkIds();
+        EXPECT_EQ(iterator->Count(), canStorePanC ? 3u : 2u);
+        iterator->Release();
+    }
+}
+
 } // namespace

src/app/tests/TestDefaultThreadNetworkDirectoryStorage.cpp

@@ -162,6 +162,8 @@ static bool IsMemoryOnlyGlobalKey(NSString * key)
     // We do not expect to see the "g/icd/cic" key; that's only used for an ICD
     // that sends check-in messages.
 
+    // We do not expect to see the "g/tnd/*" Thread Network Directory keys.
+
     return false;
 }
 

src/darwin/Framework/CHIP/MTRDemuxingStorage.mm

@@ -134,6 +134,11 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
      */
     virtual void AddPoisonKey(const std::string & key) { mPoisonKeys.insert(key); }
 
+    /**
+     * Allows subsequent writes to be rejected for unit testing purposes.
+     */
+    virtual void SetRejectWrites(bool rejectWrites) { mRejectWrites = rejectWrites; }
+
     /**
      * @brief Clear all "poison keys"
      *
@@ -233,8 +238,8 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
 
     virtual CHIP_ERROR SyncSetKeyValueInternal(const char * key, const void * value, uint16_t size)
     {
-        // Make sure poison keys are not accessed
-        if (mPoisonKeys.find(std::string(key)) != mPoisonKeys.end())
+        // Make sure writes are allowed and poison keys are not accessed
+        if (mRejectWrites || mPoisonKeys.find(std::string(key)) != mPoisonKeys.end())
         {
             return CHIP_ERROR_PERSISTED_STORAGE_FAILED;
         }
@@ -259,8 +264,8 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
 
     virtual CHIP_ERROR SyncDeleteKeyValueInternal(const char * key)
     {
-        // Make sure poison keys are not accessed
-        if (mPoisonKeys.find(std::string(key)) != mPoisonKeys.end())
+        // Make sure writes are allowed and poison keys are not accessed
+        if (mRejectWrites || mPoisonKeys.find(std::string(key)) != mPoisonKeys.end())
         {
             return CHIP_ERROR_PERSISTED_STORAGE_FAILED;
         }
@@ -273,6 +278,7 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
 
     std::map<std::string, std::vector<uint8_t>> mStorage;
     std::set<std::string> mPoisonKeys;
+    bool mRejectWrites         = false;
     LoggingLevel mLoggingLevel = LoggingLevel::kDisabled;
 };
 

src/lib/support/TestPersistentStorageDelegate.h

@@ -121,6 +121,9 @@ class OperationalDataset
      * This method returns a const ByteSpan to the extended PAN ID in the dataset.
      * This can be used to pass the extended PAN ID to a cluster command without the use of external memory.
      *
+     * Note: The returned span points into storage managed by this class,
+     * and must not be dereferenced beyond the lifetime of this object.
+     *
      * @param[out]  span  A reference to receive the location of the extended PAN ID.
      *
      * @retval CHIP_NO_ERROR                    Successfully retrieved the extended PAN ID.
@@ -256,6 +259,10 @@ class OperationalDataset
 
     /**
      * Returns ByteSpan pointing to the channel mask within the dataset.
+     *
+     * Note: The returned span points into storage managed by this class,
+     * and must not be dereferenced beyond the lifetime of this object.
+     *
      * @retval CHIP_NO_ERROR on success.
      * @retval CHIP_ERROR_TLV_TAG_NOT_FOUND if the channel mask is not present in the dataset.
      * @retval CHIP_ERROR_INVALID_TLV_ELEMENT if the TLV element is invalid.
@@ -264,13 +271,15 @@ class OperationalDataset
 
     /**
      * This method sets the channel mask within the dataset.
+     *
      * @retval CHIP_NO_ERROR on success.
      * @retval CHIP_ERROR_NO_MEMORY if there is insufficient space within the dataset.
      */
     CHIP_ERROR SetChannelMask(ByteSpan aChannelMask);
 
     /**
      * Retrieves the security policy from the dataset.
+     *
      * @retval CHIP_NO_ERROR on success.
      * @retval CHIP_ERROR_TLV_TAG_NOT_FOUND if no security policy is present in the dataset.
      * @retval CHIP_ERROR_INVALID_TLV_ELEMENT if the TLV element is invalid.
@@ -279,6 +288,7 @@ class OperationalDataset
 
     /**
      * This method sets the security policy within the dataset.
+     *
      * @retval CHIP_NO_ERROR on success.
      * @retval CHIP_ERROR_NO_MEMORY if there is insufficient space within the dataset.
      */
@@ -291,7 +301,6 @@ class OperationalDataset
 
     /**
      * This method checks if the dataset is ready for creating Thread network.
-     *
      */
     bool IsCommissioned(void) const;
 
@@ -304,7 +313,6 @@ class OperationalDataset
      * This method checks whether @p aData is formatted as ThreadTLVs.
      *
      * @note This method doesn't verify ThreadTLV values are valid.
-     *
      */
     static bool IsValid(ByteSpan aData);