From 9134d2d1b0bdef1b800b4646f525a8358b198b36 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 8 Feb 2023 13:35:53 -0500 Subject: [PATCH] Simplify and future-proof enum range checks in door locks. (#24879) * Simplify and future-proof enum range checks in door locks. Instead of explicitly checking for the min/max values, check for the "unknown value" value that decoding will decode to if the value is out of range. Fixes https://github.com/project-chip/connectedhomeip/issues/20936 * Address review commment. * Address more review comments. --- .../door-lock-server/door-lock-server.cpp | 14 ++++++-------- src/app/data-model/Nullable.h | 4 +++- src/lib/core/Optional.h | 2 ++ src/lib/core/tests/TestOptional.cpp | 19 +++++++++++++++++++ 4 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/app/clusters/door-lock-server/door-lock-server.cpp b/src/app/clusters/door-lock-server/door-lock-server.cpp index 6fb25e34e3d02c..365990cd4c9cc4 100644 --- a/src/app/clusters/door-lock-server/door-lock-server.cpp +++ b/src/app/clusters/door-lock-server/door-lock-server.cpp @@ -380,11 +380,10 @@ void DoorLockServer::setUserCommandHandler(chip::app::CommandHandler * commandOb return; } - if (!userType.IsNull() && - (userType.Value() < UserTypeEnum::kUnrestrictedUser || userType.Value() > UserTypeEnum::kRemoteOnlyUser)) + if (userType == UserTypeEnum::kUnknownEnumValue) { emberAfDoorLockClusterPrintln( - "[SetUser] Unable to set the user: user type is out of range [endpointId=%d,userIndex=%d,userType=%u]", + "[SetUser] Unable to set the user: user type is unknown [endpointId=%d,userIndex=%d,userType=%u]", commandPath.mEndpointId, userIndex, to_underlying(userType.Value())); sendClusterResponse(commandObj, commandPath, EMBER_ZCL_STATUS_INVALID_COMMAND); @@ -694,10 +693,9 @@ void DoorLockServer::setCredentialCommandHandler( return; } - if (!userType.IsNull() && - (userType.Value() < UserTypeEnum::kUnrestrictedUser || userType.Value() > UserTypeEnum::kRemoteOnlyUser)) + if (userType == UserTypeEnum::kUnknownEnumValue) { - emberAfDoorLockClusterPrintln("[SetCredential] Unable to set the credential: user type is out of range " + emberAfDoorLockClusterPrintln("[SetCredential] Unable to set the credential: user type is unknown " "[endpointId=%d,credentialIndex=%d,userType=%u]", commandPath.mEndpointId, credentialIndex, to_underlying(userType.Value())); sendSetCredentialResponse(commandObj, commandPath, DlStatus::kInvalidField, 0, nextAvailableCredentialSlot); @@ -3113,9 +3111,9 @@ void DoorLockServer::setHolidayScheduleCommandHandler(chip::app::CommandHandler return; } - if (operatingMode > OperatingModeEnum::kPassage || operatingMode < OperatingModeEnum::kNormal) + if (operatingMode == OperatingModeEnum::kUnknownEnumValue) { - emberAfDoorLockClusterPrintln("[SetHolidaySchedule] Unable to add schedule - operating mode is out of range" + emberAfDoorLockClusterPrintln("[SetHolidaySchedule] Unable to add schedule - operating mode is unknown" "[endpointId=%d,scheduleIndex=%d,localStarTime=%" PRIu32 ",localEndTime=%" PRIu32 ", operatingMode=%d]", endpointId, holidayIndex, localStartTime, localEndTime, to_underlying(operatingMode)); diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index 46586e0742f328..844a7c1b385ae9 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -83,7 +83,9 @@ struct Nullable : protected Optional static constexpr bool kIsFabricScoped = false; bool operator==(const Nullable & other) const { return Optional::operator==(other); } - bool operator!=(const Nullable & other) const { return !(*this == other); } + bool operator!=(const Nullable & other) const { return Optional::operator!=(other); } + bool operator==(const T & other) const { return Optional::operator==(other); } + bool operator!=(const T & other) const { return Optional::operator!=(other); } }; template diff --git a/src/lib/core/Optional.h b/src/lib/core/Optional.h index 316473892a3938..4edae30093c038 100644 --- a/src/lib/core/Optional.h +++ b/src/lib/core/Optional.h @@ -184,6 +184,8 @@ class Optional return (mHasValue == other.mHasValue) && (!other.mHasValue || (mValue.mData == other.mValue.mData)); } bool operator!=(const Optional & other) const { return !(*this == other); } + bool operator==(const T & other) const { return HasValue() && Value() == other; } + bool operator!=(const T & other) const { return !(*this == other); } /** Convenience method to create an optional without a valid value. */ static Optional Missing() { return Optional(); } diff --git a/src/lib/core/tests/TestOptional.cpp b/src/lib/core/tests/TestOptional.cpp index 9eff4dce870e45..af4617856894c8 100644 --- a/src/lib/core/tests/TestOptional.cpp +++ b/src/lib/core/tests/TestOptional.cpp @@ -45,6 +45,8 @@ struct Count Count(Count && o) : m(o.m) { ++created; } Count & operator=(Count &&) = default; + bool operator==(const Count & o) const { return m == o.m; } + int m; static void ResetCounter() @@ -74,26 +76,43 @@ int Count::destroyed; static void TestBasic(nlTestSuite * inSuite, void * inContext) { + // Set up our test Count objects, which will mess with counts, before we reset the + // counts. + Count c100(100), c101(101), c102(102); + Count::ResetCounter(); { auto testOptional = Optional::Value(100); NL_TEST_ASSERT(inSuite, Count::created == 1 && Count::destroyed == 0); NL_TEST_ASSERT(inSuite, testOptional.HasValue() && testOptional.Value().m == 100); + NL_TEST_ASSERT(inSuite, testOptional == c100); + NL_TEST_ASSERT(inSuite, testOptional != c101); + NL_TEST_ASSERT(inSuite, testOptional != c102); testOptional.ClearValue(); NL_TEST_ASSERT(inSuite, Count::created == 1 && Count::destroyed == 1); NL_TEST_ASSERT(inSuite, !testOptional.HasValue()); + NL_TEST_ASSERT(inSuite, testOptional != c100); + NL_TEST_ASSERT(inSuite, testOptional != c101); + NL_TEST_ASSERT(inSuite, testOptional != c102); testOptional.SetValue(Count(101)); NL_TEST_ASSERT(inSuite, Count::created == 3 && Count::destroyed == 2); NL_TEST_ASSERT(inSuite, testOptional.HasValue() && testOptional.Value().m == 101); + NL_TEST_ASSERT(inSuite, testOptional != c100); + NL_TEST_ASSERT(inSuite, testOptional == c101); + NL_TEST_ASSERT(inSuite, testOptional != c102); testOptional.Emplace(102); NL_TEST_ASSERT(inSuite, Count::created == 4 && Count::destroyed == 3); NL_TEST_ASSERT(inSuite, testOptional.HasValue() && testOptional.Value().m == 102); + NL_TEST_ASSERT(inSuite, testOptional != c100); + NL_TEST_ASSERT(inSuite, testOptional != c101); + NL_TEST_ASSERT(inSuite, testOptional == c102); } + // Our test Count objects are still in scope here. NL_TEST_ASSERT(inSuite, Count::created == 4 && Count::destroyed == 4); }