From ba490f6447d9fdc2aa8e6e20748b63bd7718d4f9 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 10 Aug 2023 10:08:35 -0400 Subject: [PATCH] Add missed code review comments (#28626) * Fix KeySetRemove to fail on key set index 0 Problem: - Removing KeySet index 0 is not allowed by spec, but SDK allowed it. - Checking that we ran without accessing fabric is not done, so error is FAILURE instead of UNSUPPORTED_ACCESS. - Fixes #28518 This PR: - Adds the check and tests against removing fabric index zero - Improves error reporting for several errors in the cluster - Combines validation logic for accessing fabric that was missing Testing: - Added unit tests and integration tests for additions (except for the unsupported access that requires PASE to check) * Regen tests with ZAP * Restyled by clang-format * Remove explicit check for undefined fabric index * Fix build * Rename ValidateAndGetProviderAndFabric to just GetProviderAndFabric * Added back cleanup for VerifyOrDie argument checking * Restyle * Update based on code review comments * Remove two more flight check comments * Restyle * Fix merge error --------- Co-authored-by: tennessee.carmelveilleux@gmail.com Co-authored-by: Restyled.io Co-authored-by: Andrei Litvin --- src/app/CommandHandler.cpp | 10 +++++++++- .../group-key-mgmt-server/group-key-mgmt-server.cpp | 4 ---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index df58e38c5129b1..d291d8154d421f 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -484,7 +484,15 @@ void CommandHandler::AddStatusAndLogIfFailure(const ConcreteCommandPath & aComma ChipLogValueIMStatus(aStatus), aMessage); } - LogErrorOnFailure(AddStatus(aCommandPath, aStatus)); + CHIP_ERROR err = AddStatus(aCommandPath, aStatus); + if (err != CHIP_NO_ERROR) + { + ChipLogError(DataManagement, + "Failed to set status on Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI + ": %" CHIP_ERROR_FORMAT, + aCommandPath.mEndpointId, ChipLogValueMEI(aCommandPath.mClusterId), ChipLogValueMEI(aCommandPath.mCommandId), + err.Format()); + } } CHIP_ERROR CommandHandler::AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus) diff --git a/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp b/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp index 51a9977977a042..fcd0dfa1353516 100644 --- a/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp +++ b/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp @@ -450,7 +450,6 @@ bool emberAfGroupKeyManagementClusterKeySetWriteCallback( Credentials::GroupDataProvider * provider = nullptr; const FabricInfo * fabric = nullptr; - // Flight-check fabric scoping. if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric)) { // Command will already have status populated from validation. @@ -555,7 +554,6 @@ bool emberAfGroupKeyManagementClusterKeySetReadCallback( Credentials::GroupDataProvider * provider = nullptr; const FabricInfo * fabric = nullptr; - // Flight-check fabric scoping. if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric)) { // Command will already have status populated from validation. @@ -621,7 +619,6 @@ bool emberAfGroupKeyManagementClusterKeySetRemoveCallback( Credentials::GroupDataProvider * provider = nullptr; const FabricInfo * fabric = nullptr; - // Flight-check fabric scoping. if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric)) { // Command will already have status populated from validation. @@ -694,7 +691,6 @@ bool emberAfGroupKeyManagementClusterKeySetReadAllIndicesCallback( Credentials::GroupDataProvider * provider = nullptr; const FabricInfo * fabric = nullptr; - // Flight-check fabric scoping. if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric)) { // Command will already have status populated from validation.