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.

Sign up for GitHub

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

Jump to bottom

lib: smf: add NULL check in smf_execute_all_exit_actions #73632

Merged
merged 2 commits into from
Jun 12, 2024
Merged

lib: smf: add NULL check in smf_execute_all_exit_actions #73632

merged 2 commits into from
Jun 12, 2024
+7 −5

Conversation

ccli8
Copy link
Contributor

@ccli8 ccli8 commented Jun 3, 2024

Like in smf_execute_all_entry_actions(), add NULL check in smf_execute_all_exit_actions().

@zephyrbot zephyrbot added the area: State Machine Framework State Machine Framework label Jun 3, 2024
@ccli8
lib: smf: add NULL check in smf_execute_all_exit_actions
811a974 
Like in smf_execute_all_entry_actions(), add NULL check
in smf_execute_all_exit_actions().

Signed-off-by: Chun-Chieh Li <ccli8@nuvoton.com>
keith-zephyr
keith-zephyr previously approved these changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@keith-zephyr keith-zephyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. It would be appreciated if you could extend one of the existing test cases to verify this behavior.

@keith-zephyr
Copy link
Contributor

Tagging @glenn-andrews for your visibility. If you want to nominate yourself as a collaborator on the SMF, you will automatically get added to PRs that touch the SMF.

@glenn-andrews
Copy link
Collaborator

What was the situation where this error occurred?

topmost should be a valid state, as should ctx->current and all parents between current and topmost.

We may have a deeper error with determining topmost?

@glenn-andrews
Copy link
Collaborator

Tagging @glenn-andrews for your visibility. If you want to nominate yourself as a collaborator on the SMF, you will automatically get added to PRs that touch the SMF.

Done: #73729

@ccli8
Copy link
Contributor Author

ccli8 commented Jun 5, 2024

Per my check, topmost becomes invalid because ctx->executing is not updated in smf_execute_all_entry_actions. I add one more commit to fix it.

@ccli8
Copy link
Contributor Author

ccli8 commented Jun 6, 2024

addressed tests/lib/smf test failure

@glenn-andrews
Copy link
Collaborator

Per my check, topmost becomes invalid because ctx->executing is not updated in smf_execute_all_entry_actions. I add one more commit to fix it.

Good find. With this fix, is the null check still needed?

@ccli8
Copy link
Contributor Author

ccli8 commented Jun 7, 2024

Good find. With this fix, is the null check still needed?

It becomes unnecessary. I keep it there because it does no harm. If concerned, I can remove the null check.

keith-zephyr
keith-zephyr reviewed Jun 7, 2024
View reviewed changes
lib/smf/smf.c Outdated
@@ -98,12 +98,12 @@ static bool smf_execute_all_entry_actions(struct smf_ctx *const ctx,
for (const struct smf_state *to_execute = get_child_of(new_state, topmost);
to_execute != NULL && to_execute != new_state;
to_execute = get_child_of(new_state, to_execute)) {
/* Keep track of the executing entry action in case it calls
* smf_set_State()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* smf_set_State()
* smf_set_state()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@glenn-andrews
Copy link
Collaborator

glenn-andrews commented Jun 7, 2024 •
edited
Loading

Good find. With this fix, is the null check still needed?

It becomes unnecessary. I keep it there because it does no harm. If concerned, I can remove the null check.

It adds another check to an already-long if statement, and gets called for every parent. But that's still trivial compared to the CPU cycles we waste trying to find the LCA before this function gets called.

Feel free to leave it in.

@ccli8
lib: smf: fix ctx->executing in smf_execute_all_entry_actions
cf922f2 
Fix ctx->executing is not updated in smf_execute_all_entry_actions()

Signed-off-by: Chun-Chieh Li <ccli8@nuvoton.com>
@ccli8
Copy link
Contributor Author

ccli8 commented Jun 11, 2024

Feel free to leave it in.

I keep the null check there if no other concern.

glenn-andrews
glenn-andrews approved these changes Jun 11, 2024
View reviewed changes
Copy link
Collaborator

@glenn-andrews glenn-andrews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@nashif nashif merged commit 55a988e into zephyrproject-rtos:main Jun 12, 2024
22 checks passed
@ccli8 ccli8 deleted the smf_fix_deref_null branch June 13, 2024 01:19
@ccli8 ccli8 mentioned this pull request Jun 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glenn-andrews glenn-andrews glenn-andrews approved these changes

@sambhurst sambhurst sambhurst approved these changes

@keith-zephyr keith-zephyr keith-zephyr approved these changes

Assignees

@sambhurst sambhurst

Labels
area: State Machine Framework State Machine Framework
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ccli8 @keith-zephyr @glenn-andrews @sambhurst @nashif @zephyrbot