From 1682070b4c9e851f02b70b66f07c3cdedbb12f88 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Damian=20Kr=C3=B3lik?=
 <66667989+Damian-Nordic@users.noreply.github.com>
Date: Thu, 4 Aug 2022 13:30:17 +0200
Subject: [PATCH] [OpenThread] Revert configuration on boot with fail-safe
 armed (#21597)

* [OpenThread] Revert configuration on boot with fail-safe armed

After recent changes related to the fail-safe commit marker
persistence RevertConfiguration() is no longer called by the
common code when the device reboots with the fail-safe armed.

Due to the nature of OpenThread API, all network credential
changes are persistent, so an explicit revert is needed in
such a case.

By the way, remove the workaround for KVS not being able to
store empty values as the scenario has already been verified
by the storage audit.

* Address code review comment
---
 ...enericNetworkCommissioningThreadDriver.cpp | 40 +++++++++----------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp b/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp
index 92a390625410c2..c4ff7934d12a1d 100644
--- a/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp
+++ b/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp
@@ -43,17 +43,15 @@ namespace NetworkCommissioning {
 // KVS slot upon any changes and restored when the fail-safe timeout is triggered or the device
 // reboots without completing all the changes.
 
-// Not all KVS implementations support zero-length values, so use this special value, that is not a valid
-// dataset, to represent an empty dataset. We need that to be able to revert the network configuration
-// in the case of an unsuccessful commissioning.
-constexpr uint8_t kEmptyDataset[1] = {};
-
 CHIP_ERROR GenericThreadDriver::Init(Internal::BaseDriver::NetworkStatusChangeCallback * statusChangeCallback)
 {
     ThreadStackMgrImpl().SetNetworkStatusChangeCallback(statusChangeCallback);
+    ThreadStackMgrImpl().GetThreadProvision(mStagingNetwork);
 
-    VerifyOrReturnError(ThreadStackMgrImpl().IsThreadAttached(), CHIP_NO_ERROR);
-    VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(mStagingNetwork) == CHIP_NO_ERROR, CHIP_NO_ERROR);
+    // If the network configuration backup exists, it means that the device has been rebooted with
+    // the fail-safe armed. Since OpenThread persists all operational dataset changes, the backup
+    // must be restored on the boot. If there's no backup, the below function is a no-op.
+    RevertConfiguration();
 
     return CHIP_NO_ERROR;
 }
@@ -86,23 +84,23 @@ CHIP_ERROR GenericThreadDriver::RevertConfiguration()
     // If no backup could be found, it means that the network configuration has not been modified
     // since the fail-safe was armed, so return with no error.
     ReturnErrorCodeIf(error == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, CHIP_NO_ERROR);
-    ReturnErrorOnFailure(error);
 
-    ChipLogError(NetworkProvisioning, "Found Thread configuration backup: reverting configuration");
+    ChipLogProgress(NetworkProvisioning, "Reverting Thread operational dataset");
 
-    // Not all KVS implementations support zero-length values, so handle a special value representing an empty dataset.
-    ByteSpan dataset(datasetBytes, datasetLength);
+    if (error == CHIP_NO_ERROR)
+    {
+        error = mStagingNetwork.Init(ByteSpan(datasetBytes, datasetLength));
+    }
 
-    if (dataset.data_equal(ByteSpan(kEmptyDataset)))
+    if (error == CHIP_NO_ERROR)
     {
-        dataset = {};
+        error = DeviceLayer::ThreadStackMgrImpl().AttachToThreadNetwork(mStagingNetwork, /* callback */ nullptr);
     }
 
-    ReturnErrorOnFailure(mStagingNetwork.Init(dataset));
-    ReturnErrorOnFailure(DeviceLayer::ThreadStackMgrImpl().AttachToThreadNetwork(mStagingNetwork, /* callback */ nullptr));
+    // Always remove the backup, regardless if it can be successfully restored.
+    KeyValueStoreMgr().Delete(key.FailSafeNetworkConfig());
 
-    // TODO: What happens on errors above? Why do we not remove the failsafe?
-    return KeyValueStoreMgr().Delete(key.FailSafeNetworkConfig());
+    return error;
 }
 
 Status GenericThreadDriver::AddOrUpdateNetwork(ByteSpan operationalDataset, MutableCharSpan & outDebugText,
@@ -203,16 +201,16 @@ Status GenericThreadDriver::MatchesNetworkId(const Thread::OperationalDataset &
 CHIP_ERROR GenericThreadDriver::BackupConfiguration()
 {
     DefaultStorageKeyAllocator key;
-    uint8_t dummy;
 
     // If configuration is already backed up, return with no error
-    if (KeyValueStoreMgr().Get(key.FailSafeNetworkConfig(), &dummy, 0) == CHIP_ERROR_BUFFER_TOO_SMALL)
+    CHIP_ERROR err = KeyValueStoreMgr().Get(key.FailSafeNetworkConfig(), nullptr, 0);
+
+    if (err == CHIP_NO_ERROR || err == CHIP_ERROR_BUFFER_TOO_SMALL)
     {
         return CHIP_NO_ERROR;
     }
 
-    // Not all KVS implementations support zero-length values, so use a special value in such a case.
-    ByteSpan dataset = mStagingNetwork.IsEmpty() ? ByteSpan(kEmptyDataset) : mStagingNetwork.AsByteSpan();
+    ByteSpan dataset = mStagingNetwork.AsByteSpan();
 
     return KeyValueStoreMgr().Put(key.FailSafeNetworkConfig(), dataset.data(), dataset.size());
 }