Use pointer for subject descriptor in datamodel provider instead of std::optional (#36246)

andy31415 · andreilitvin · web-flow · commit 21428707a8fe · 2024-10-28T13:07:31.000Z
* Use a pointer for the subject descriptor.

This seems to save about 88 bytes of flash on a test NRF board.

* Restyle

* Fix include

* Fix include

* Also fix PW rpc

---------

Co-authored-by: Andrei Litvin &lt;andreilitvin@google.com&gt;
diff --git a/examples/common/pigweed/rpc_services/Attributes.h b/examples/common/pigweed/rpc_services/Attributes.h
@@ -224,7 +224,7 @@ class Attributes : public pw_rpc::nanopb::Attributes::Service<Attributes>
         app::DataModel::ReadAttributeRequest request;
         request.path = path;
         request.operationFlags.Set(app::DataModel::OperationFlags::kInternal);
-        request.subjectDescriptor = subjectDescriptor;
+        request.subjectDescriptor = &subjectDescriptor;
 
         std::optional<app::DataModel::ClusterInfo> info = provider->GetClusterInfo(path);
         if (!info.has_value())
diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp
@@ -18,6 +18,7 @@
 #include <app/CommandHandlerImpl.h>
 
 #include <access/AccessControl.h>
+#include <access/SubjectDescriptor.h>
 #include <app-common/zap-generated/cluster-objects.h>
 #include <app/MessageDef/StatusIB.h>
 #include <app/RequiredPrivilege.h>
@@ -391,10 +392,11 @@ Status CommandHandlerImpl::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
     VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);
 
     {
+        Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor();
         DataModel::InvokeRequest request;
 
         request.path              = concretePath;
-        request.subjectDescriptor = GetSubjectDescriptor();
+        request.subjectDescriptor = &subjectDescriptor;
         request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, IsTimedInvoke());
 
         Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request);
@@ -513,10 +515,11 @@ Status CommandHandlerImpl::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo
         const ConcreteCommandPath concretePath(mapping.endpoint_id, clusterId, commandId);
 
         {
+            Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor();
             DataModel::InvokeRequest request;
 
             request.path              = concretePath;
-            request.subjectDescriptor = GetSubjectDescriptor();
+            request.subjectDescriptor = &subjectDescriptor;
             request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, IsTimedInvoke());
 
             Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request);
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -1646,10 +1646,12 @@ void InteractionModelEngine::DispatchCommand(CommandHandlerImpl & apCommandObj,
 {
 #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
 
+    Access::SubjectDescriptor subjectDescriptor = apCommandObj.GetSubjectDescriptor();
+
     DataModel::InvokeRequest request;
     request.path = aCommandPath;
     request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, apCommandObj.IsTimedInvoke());
-    request.subjectDescriptor = apCommandObj.GetSubjectDescriptor();
+    request.subjectDescriptor = &subjectDescriptor;
 
     std::optional<DataModel::ActionReturnStatus> status = GetDataModelProvider()->Invoke(request, apPayload, &apCommandObj);
 
@@ -1702,7 +1704,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::ValidateCommandCanBe
 
 Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandAccess(const DataModel::InvokeRequest & aRequest)
 {
-    if (!aRequest.subjectDescriptor.has_value())
+    if (aRequest.subjectDescriptor == nullptr)
     {
         return Status::UnsupportedAccess; // we require a subject for invoke
     }
diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp
@@ -779,7 +779,7 @@ CHIP_ERROR WriteHandler::WriteClusterData(const Access::SubjectDescriptor & aSub
     DataModel::WriteAttributeRequest request;
 
     request.path                = aPath;
-    request.subjectDescriptor   = aSubject;
+    request.subjectDescriptor   = &aSubject;
     request.previousSuccessPath = mLastSuccessfullyWrittenPath;
     request.writeFlags.Set(DataModel::WriteFlags::kTimed, IsTimedWrite());
 
diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp
@@ -106,7 +106,7 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::ReadAttribute(const Data
     // ACL check for non-internal requests
     if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal))
     {
-        VerifyOrReturnError(request.subjectDescriptor.has_value(), CHIP_ERROR_INVALID_ARGUMENT);
+        VerifyOrReturnError(request.subjectDescriptor != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 
         Access::RequestPath requestPath{ .cluster     = request.path.mClusterId,
                                          .endpoint    = request.path.mEndpointId,
diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp
@@ -159,7 +159,7 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat
 
     if (checkAcl)
     {
-        VerifyOrReturnError(request.subjectDescriptor.has_value(), Status::UnsupportedAccess);
+        VerifyOrReturnError(request.subjectDescriptor != nullptr, Status::UnsupportedAccess);
 
         Access::RequestPath requestPath{ .cluster     = request.path.mClusterId,
                                          .endpoint    = request.path.mEndpointId,
diff --git a/src/app/data-model-provider/OperationTypes.h b/src/app/data-model-provider/OperationTypes.h
@@ -55,15 +55,16 @@ struct OperationRequest
     ///  - operationFlags.Has(OperationFlags::kInternal) MUST NOT have this set
     ///
     /// NOTE: once kInternal flag is removed, this will become non-optional
-    std::optional<chip::Access::SubjectDescriptor> subjectDescriptor;
+    const chip::Access::SubjectDescriptor * subjectDescriptor = nullptr;
 
     /// Accessing fabric index is the subjectDescriptor fabric index (if any).
     /// This is a readability convenience function.
     ///
     /// Returns kUndefinedFabricIndex if no subject descriptor is available
     FabricIndex GetAccessingFabricIndex() const
     {
-        return subjectDescriptor.has_value() ? subjectDescriptor->fabricIndex : kUndefinedFabricIndex;
+        VerifyOrReturnValue(subjectDescriptor != nullptr, kUndefinedFabricIndex);
+        return subjectDescriptor->fabricIndex;
     }
 };
 
diff --git a/src/app/data-model-provider/tests/ReadTesting.h b/src/app/data-model-provider/tests/ReadTesting.h
@@ -136,7 +136,7 @@ class ReadOperation
     ReadOperation(const ConcreteAttributePath & path)
     {
         mRequest.path              = path;
-        mRequest.subjectDescriptor = kDenySubjectDescriptor;
+        mRequest.subjectDescriptor = &kDenySubjectDescriptor;
     }
 
     ReadOperation(EndpointId endpoint, ClusterId cluster, AttributeId attribute) :
@@ -146,7 +146,7 @@ class ReadOperation
     ReadOperation & SetSubjectDescriptor(const chip::Access::SubjectDescriptor & descriptor)
     {
         VerifyOrDie(mState == State::kInitializing);
-        mRequest.subjectDescriptor = descriptor;
+        mRequest.subjectDescriptor = &descriptor;
         return *this;
     }
 
diff --git a/src/app/data-model-provider/tests/WriteTesting.h b/src/app/data-model-provider/tests/WriteTesting.h
@@ -47,7 +47,7 @@ class WriteOperation
     WriteOperation(const ConcreteDataAttributePath & path)
     {
         mRequest.path              = path;
-        mRequest.subjectDescriptor = kDenySubjectDescriptor;
+        mRequest.subjectDescriptor = &kDenySubjectDescriptor;
     }
 
     WriteOperation(EndpointId endpoint, ClusterId cluster, AttributeId attribute) :
@@ -56,7 +56,7 @@ class WriteOperation
 
     WriteOperation & SetSubjectDescriptor(const chip::Access::SubjectDescriptor & descriptor)
     {
-        mRequest.subjectDescriptor = descriptor;
+        mRequest.subjectDescriptor = &descriptor;
         return *this;
     }
 
@@ -123,7 +123,11 @@ class WriteOperation
     AttributeValueDecoder DecoderFor(const T & value)
     {
         mTLVReader = ReadEncodedValue(value);
-        return AttributeValueDecoder(mTLVReader, mRequest.subjectDescriptor.value_or(kDenySubjectDescriptor));
+        if (mRequest.subjectDescriptor == nullptr)
+        {
+            AttributeValueDecoder(mTLVReader, kDenySubjectDescriptor);
+        }
+        return AttributeValueDecoder(mTLVReader, *mRequest.subjectDescriptor);
     }
 
 private:
diff --git a/src/app/reporting/Read-DataModel.cpp b/src/app/reporting/Read-DataModel.cpp
@@ -47,7 +47,7 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode
     {
         readRequest.readFlags.Set(DataModel::ReadFlags::kFabricFiltered);
     }
-    readRequest.subjectDescriptor = subjectDescriptor;
+    readRequest.subjectDescriptor = &subjectDescriptor;
     readRequest.path              = path;
 
     DataVersion version = 0;
diff --git a/src/app/tests/test-interaction-model-api.cpp b/src/app/tests/test-interaction-model-api.cpp
@@ -13,6 +13,7 @@
  *    See the License for the specific language governing permissions and
  *    limitations under the License.
  */
+#include "access/SubjectDescriptor.h"
 #include <app/tests/test-interaction-model-api.h>
 
 #include <app/InteractionModelEngine.h>
@@ -171,8 +172,13 @@ ActionReturnStatus TestImCustomDataModel::ReadAttribute(const ReadAttributeReque
 {
     AttributeEncodeState mutableState(&encoder.GetState()); // provide a state copy to start.
 
-    CHIP_ERROR err = ReadSingleClusterData(request.subjectDescriptor.value_or(Access::SubjectDescriptor()),
-                                           request.readFlags.Has(ReadFlags::kFabricFiltered), request.path,
+    Access::SubjectDescriptor subjectDescriptor;
+    if (request.subjectDescriptor != nullptr)
+    {
+        subjectDescriptor = *request.subjectDescriptor;
+    }
+
+    CHIP_ERROR err = ReadSingleClusterData(subjectDescriptor, request.readFlags.Has(ReadFlags::kFabricFiltered), request.path,
                                            TestOnlyAttributeValueEncoderAccessor(encoder).Builder(), &mutableState);
 
     // state must survive CHIP_ERRORs as it is used for chunking
diff --git a/src/controller/tests/data_model/DataModelFixtures.cpp b/src/controller/tests/data_model/DataModelFixtures.cpp
@@ -18,6 +18,7 @@
 
 #include "DataModelFixtures.h"
 
+#include <access/SubjectDescriptor.h>
 #include <app-common/zap-generated/cluster-objects.h>
 #include <app-common/zap-generated/ids/Clusters.h>
 #include <app/AttributeValueDecoder.h>
@@ -522,8 +523,13 @@ ActionReturnStatus CustomDataModel::ReadAttribute(const ReadAttributeRequest & r
     }
 #endif // CHIP_CONFIG_USE_EMBER_DATA_MODEL && CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
 
-    CHIP_ERROR err = ReadSingleClusterData(request.subjectDescriptor.value_or(Access::SubjectDescriptor()),
-                                           request.readFlags.Has(ReadFlags::kFabricFiltered), request.path,
+    Access::SubjectDescriptor subjectDescriptor;
+    if (request.subjectDescriptor != nullptr)
+    {
+        subjectDescriptor = *request.subjectDescriptor;
+    }
+
+    CHIP_ERROR err = ReadSingleClusterData(subjectDescriptor, request.readFlags.Has(ReadFlags::kFabricFiltered), request.path,
                                            TestOnlyAttributeValueEncoderAccessor(encoder).Builder(), &mutableState);
 
     // state must survive CHIP_ERRORs as it is used for chunking

Original file line number	Diff line number	Diff line change
`@@ -1646,10 +1646,12 @@ void InteractionModelEngine::DispatchCommand(CommandHandlerImpl & apCommandObj,`
`1646`	`1646`	`{`
`1647`	`1647`	`#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE`
`1648`	`1648`
	`1649`	`+ Access::SubjectDescriptor subjectDescriptor = apCommandObj.GetSubjectDescriptor();`
	`1650`	`+`
`1649`	`1651`	`DataModel::InvokeRequest request;`
`1650`	`1652`	`request.path = aCommandPath;`
`1651`	`1653`	`request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, apCommandObj.IsTimedInvoke());`
`1652`		`- request.subjectDescriptor = apCommandObj.GetSubjectDescriptor();`
	`1654`	`+ request.subjectDescriptor = &subjectDescriptor;`
`1653`	`1655`
`1654`	`1656`	`std::optional<DataModel::ActionReturnStatus> status = GetDataModelProvider()->Invoke(request, apPayload, &apCommandObj);`
`1655`	`1657`
`@@ -1702,7 +1704,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::ValidateCommandCanBe`
`1702`	`1704`
`1703`	`1705`	`Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandAccess(const DataModel::InvokeRequest & aRequest)`
`1704`	`1706`	`{`
`1705`		`- if (!aRequest.subjectDescriptor.has_value())`
	`1707`	`+ if (aRequest.subjectDescriptor == nullptr)`
`1706`	`1708`	`{`
`1707`	`1709`	`return Status::UnsupportedAccess; // we require a subject for invoke`
`1708`	`1710`	`}`
Original file line number	Diff line number	Diff line change
`@@ -106,7 +106,7 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::ReadAttribute(const Data`
`106`	`106`	`// ACL check for non-internal requests`
`107`	`107`	`if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal))`
`108`	`108`	`{`
`109`		`- VerifyOrReturnError(request.subjectDescriptor.has_value(), CHIP_ERROR_INVALID_ARGUMENT);`
	`109`	`+ VerifyOrReturnError(request.subjectDescriptor != nullptr, CHIP_ERROR_INVALID_ARGUMENT);`
`110`	`110`
`111`	`111`	`Access::RequestPath requestPath{ .cluster = request.path.mClusterId,`
`112`	`112`	`.endpoint = request.path.mEndpointId,`
Original file line number	Diff line number	Diff line change
`@@ -159,7 +159,7 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat`
`159`	`159`
`160`	`160`	`if (checkAcl)`
`161`	`161`	`{`
`162`		`- VerifyOrReturnError(request.subjectDescriptor.has_value(), Status::UnsupportedAccess);`
	`162`	`+ VerifyOrReturnError(request.subjectDescriptor != nullptr, Status::UnsupportedAccess);`
`163`	`163`
`164`	`164`	`Access::RequestPath requestPath{ .cluster = request.path.mClusterId,`
`165`	`165`	`.endpoint = request.path.mEndpointId,`
Original file line number	Diff line number	Diff line change
`@@ -136,7 +136,7 @@ class ReadOperation`
`136`	`136`	`ReadOperation(const ConcreteAttributePath & path)`
`137`	`137`	`{`
`138`	`138`	`mRequest.path = path;`
`139`		`- mRequest.subjectDescriptor = kDenySubjectDescriptor;`
	`139`	`+ mRequest.subjectDescriptor = &kDenySubjectDescriptor;`
`140`	`140`	`}`
`141`	`141`
`142`	`142`	`ReadOperation(EndpointId endpoint, ClusterId cluster, AttributeId attribute) :`
`@@ -146,7 +146,7 @@ class ReadOperation`
`146`	`146`	`ReadOperation & SetSubjectDescriptor(const chip::Access::SubjectDescriptor & descriptor)`
`147`	`147`	`{`
`148`	`148`	`VerifyOrDie(mState == State::kInitializing);`
`149`		`- mRequest.subjectDescriptor = descriptor;`
	`149`	`+ mRequest.subjectDescriptor = &descriptor;`
`150`	`150`	`return *this;`
`151`	`151`	`}`
`152`	`152`
Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,7 @@ class WriteOperation`
`47`	`47`	`WriteOperation(const ConcreteDataAttributePath & path)`
`48`	`48`	`{`
`49`	`49`	`mRequest.path = path;`
`50`		`- mRequest.subjectDescriptor = kDenySubjectDescriptor;`
	`50`	`+ mRequest.subjectDescriptor = &kDenySubjectDescriptor;`
`51`	`51`	`}`
`52`	`52`
`53`	`53`	`WriteOperation(EndpointId endpoint, ClusterId cluster, AttributeId attribute) :`
`@@ -56,7 +56,7 @@ class WriteOperation`
`56`	`56`
`57`	`57`	`WriteOperation & SetSubjectDescriptor(const chip::Access::SubjectDescriptor & descriptor)`
`58`	`58`	`{`
`59`		`- mRequest.subjectDescriptor = descriptor;`
	`59`	`+ mRequest.subjectDescriptor = &descriptor;`
`60`	`60`	`return *this;`
`61`	`61`	`}`
`62`	`62`
`@@ -123,7 +123,11 @@ class WriteOperation`
`123`	`123`	`AttributeValueDecoder DecoderFor(const T & value)`
`124`	`124`	`{`
`125`	`125`	`mTLVReader = ReadEncodedValue(value);`
`126`		`- return AttributeValueDecoder(mTLVReader, mRequest.subjectDescriptor.value_or(kDenySubjectDescriptor));`
	`126`	`+ if (mRequest.subjectDescriptor == nullptr)`
	`127`	`+ {`
	`128`	`+ AttributeValueDecoder(mTLVReader, kDenySubjectDescriptor);`
	`129`	`+ }`
	`130`	`+ return AttributeValueDecoder(mTLVReader, *mRequest.subjectDescriptor);`
`127`	`131`	`}`
`128`	`132`
`129`	`133`	`private:`
Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,7 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode`
`47`	`47`	`{`
`48`	`48`	`readRequest.readFlags.Set(DataModel::ReadFlags::kFabricFiltered);`
`49`	`49`	`}`
`50`		`- readRequest.subjectDescriptor = subjectDescriptor;`
	`50`	`+ readRequest.subjectDescriptor = &subjectDescriptor;`
`51`	`51`	`readRequest.path = path;`
`52`	`52`
`53`	`53`	`DataVersion version = 0;`