Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] AddStatus error code is often ignored #28630

Closed
andy31415 opened this issue Aug 10, 2023 · 3 comments · Fixed by #28634 or #28800
Closed

[BUG] AddStatus error code is often ignored #28630

andy31415 opened this issue Aug 10, 2023 · 3 comments · Fixed by #28634 or #28800
Assignees
Labels
bug Something isn't working needs triage

Comments

@andy31415
Copy link
Contributor

andy31415 commented Aug 10, 2023

Reproduction steps

Examples from rg -- '->AddStatus':

...
examples/ota-provider-app/ota-provider-common/OTAProviderExample.cpp:                commandObj->AddStatus(commandPath, Status::Failure);
examples/ota-provider-app/ota-provider-common/OTAProviderExample.cpp:    commandObj->AddStatus(commandPath, Status::Success);
src/controller/tests/data_model/TestCommands.cpp:            apCommandObj->AddStatusAndLogIfFailure(aCommandPath, Protocols::InteractionModel::Status::Failure,
src/controller/tests/data_model/TestCommands.cpp:            apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::Success);
src/controller/tests/data_model/TestCommands.cpp:                apCommandObj->AddStatusAndLogIfFailure(aCommandPath, Protocols::InteractionModel::Status::Succes
...
src/app/clusters/channel-server/channel-server.cpp:        command->AddStatus(commandPath, Status::Failure);
src/app/clusters/channel-server/channel-server.cpp:    command->AddStatus(commandPath, status);
src/app/clusters/channel-server/channel-server.cpp:    command->AddStatus(commandPath, status);
src/app/clusters/identify-server/identify-server.cpp:    commandObj->AddStatus(commandPath,
src/app/clusters/identify-server/identify-server.cpp:    commandObj->AddStatus(commandPath, Status::Success);
...

A large percentage of AddStatus commands are never actually checked for return codes.

This has two sideffects:

  • errors are never seen in logs
  • we have no sane way to handle AddStatus failures (would we return success without an error code?)

We likely would want to VerifyOrDie unless we are able to handle the error in some way.
Also need to review usage of AddStatusAndLogIfFailure since that both logs failure statuses (seems to be the intended functionality) but also adds a log of failed statuses (VerifyOrDie seems more correct though as we cannot really handle the failure).

@andy31415 andy31415 added bug Something isn't working needs triage labels Aug 10, 2023
@andy31415
Copy link
Contributor Author

This seems currently mitigated by the fact that AddStatus does not really fail as we only handle single statuses and they will fit inside our packets.

This also means that a VerifyOrDie approach will also likely not cause any issues on the existing codebase.

@bzbarsky-apple
Copy link
Contributor

Reopening, since #28634 was reverted....

@andy31415
Copy link
Contributor Author

Revert seems to have happened in #28661 due to OpenIOT perma-fail.

At the same time OpenIOT seems quite flaky in general (we had to disable several tests), so I am somewhat unclear on what is happening here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs triage
Projects
None yet
2 participants