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

sys/event: add assertion that event has a handler #20847

Merged
merged 5 commits into from
Oct 25, 2024
Merged

sys/event: add assertion that event has a handler #20847

merged 5 commits into from
Oct 25, 2024

Conversation

benpicco
Copy link
Contributor

Contribution description

When an uninitialized event is added, we will dereference a NULL pointer.
It can then be a bit tricky to find out which event caused this - so add an assertion when queuing the event that a callback is set.

Testing procedure

Issues/PRs references

@benpicco benpicco added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Aug 29, 2024
@github-actions github-actions bot added Area: network Area: Networking Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System Area: examples Area: Example Applications labels Aug 29, 2024
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 29, 2024
@riot-ci
Copy link

riot-ci commented Aug 29, 2024

Murdock results

✔️ PASSED

16c447a gcoap: make use of event_callback_post()

Success Failures Total Runtime
10193 0 10193 18m:19s

Artifacts

Teufelchen1
Teufelchen1 reviewed Sep 25, 2024
View reviewed changes
Copy link
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

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

Overall looks good, would you mind reminding me if I have not come around for a proper review in a couple weeks?

Teufelchen1
Teufelchen1 approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

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

LGTM. Any specific reason you omitted an assert(cb) in callback.c event_callback_init(..)?

@benpicco
Copy link
Contributor Author

A callback struct with a NULL handler does no harm in itself.
Only when you try to add it to an event queue, things will blow up.

@Teufelchen1 Teufelchen1 added this pull request to the merge queue Oct 25, 2024
Merged via the queue into RIOT-OS:master with commit b376bec Oct 25, 2024
29 checks passed
@benpicco benpicco deleted the event_assert branch October 25, 2024 11:42
@benpicco benpicco added this to the Release 2024.10 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Teufelchen1 Teufelchen1 Teufelchen1 approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

Assignees
No one assigned
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2024.10
Development

Successfully merging this pull request may close these issues.
3 participants
@benpicco @riot-ci @Teufelchen1