Validate for a valid cluster path when Invoke is called #37207
+231
−5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review changes with Changed Files
|
|
PR #37207: Size comparison from d21aaa5 to 325ac8a Full report (3 builds for cc32xx, stm32)
|
|
PR #37207: Size comparison from d21aaa5 to 31e5ee7 Full report (20 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
tcarmelveilleux
approved these changes
Jan 27, 2025
arkq
approved these changes
Jan 27, 2025
tehampson
reviewed
Jan 27, 2025
tehampson
reviewed
Jan 27, 2025
|
PR #37207: Size comparison from d21aaa5 to 63fcea0 Full report (44 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
|
PR #37207: Size comparison from d21aaa5 to e94b6a7 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
tehampson
approved these changes
Jan 27, 2025
Co-authored-by: Boris Zbarsky <[email protected]>
|
PR #37207: Size comparison from d21aaa5 to a605a2c Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| @@ -159,6 +159,16 @@ std::optional<DataModel::ActionReturnStatus> CodegenDataModelProvider::Invoke(co | |||
| TLV::TLVReader & input_arguments, | |||
| CommandHandler * handler) | |||
| { | |||
| // Ensure the command actually exists on the relevant cluster instance. | |||
There was a problem hiding this comment.
This does not in fact ensure that. This is ensuring the cluster exists. It does not ensure that the cluster instance involved exposes this command. That would involve interrogating the CHI instance to see what commands it supports for this specific cluster path.
Note that just doing InvokeCommand on the CHI might not do that check, because the invariant so far has been that InvokeCommand is only called if you claim to have the command at all....
So either we need to end up checking the AcceptedCommands thing from here anyway, or we need a change in the CHI contract....
More generally, I think we should try to minimize the amount of logic that providers have to implement (and hence get wrong). That's why we currently have the existence checks in the IM engine...
There was a problem hiding this comment.
Updated comment, I think in this case I would say "this makes it better than before" and would follow up on changing things.
From a processing logic perspective, I think we should not require invoke to accept invalid paths, but not preventing them from double-checking them either. I am a fan of trust but verify in this case.
The current contract in CHI is that commands that are not handled are silently skipped (to allow for ember fallback) so it should handle commands that are unknown. However actually double checking this is probably not practical.
|
PR #37207: Size comparison from b082219 to 7052ac5 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #37184
When we are performing an invoke, our logic allows for CommandHandlerInterface to do the first processing and if that fails, we fall back to ember. In the
SoftwareDiagnosticscase (and probably other) we register them as wildcard endpoints so we always find the CHI in that case.Existing code would do "find CHI and if found, let it process" and because SoftwareDiagnostics was available on any endpoint, we would just accept any endpoint. Added code to pre-validate we have a good endpoint/cluster combination before even attempting CHI.
Testing
Unit test and integration test added.