fix(cie_thread_configurator): address misc code quality issues#1277
Open
atsushi421 wants to merge 4 commits intomainfrom
Open
fix(cie_thread_configurator): address misc code quality issues#1277atsushi421 wants to merge 4 commits intomainfrom
atsushi421 wants to merge 4 commits intomainfrom
Conversation
Port code quality fixes from upstream callback_isolated_executor#51: - Add `inline` to `sched_setattr()` in header to prevent ODR violation - Zero-initialize `ThreadConfig` members to avoid uninitialized reads - Reject unknown scheduling policies at YAML load time - Return `false` from `issue_syscalls()` for unrecognized policy strings - Fix trailing comma in `set_affinity_by_cgroup()` cpuset.cpus output Signed-off-by: atsushi421 <atsushi.yano.2@tier4.jp>
… lookup - Zero-initialize domain_id member for consistency with other ThreadConfig fields - Use count() instead of find()==end() for cleaner set membership checks Signed-off-by: atsushi421 <atsushi.yano.2@tier4.jp>
atsushi421
added
the
need-patch-update
Contributor
There was a problem hiding this comment.
Pull request overview
Ports upstream code-quality and correctness fixes into agnocast_cie_thread_configurator, improving config validation and reducing subtle runtime/ODR issues.
Changes:
- Validate scheduling policy strings at YAML load time and reject unknown policies early.
- Fix
cpuset.cpusformatting to avoid a trailing comma. - Add
inlinetosched_setattr()and default-initializeThreadConfigfields to prevent ODR/uninitialized-read issues.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/agnocast_cie_thread_configurator/src/thread_configurator_node.cpp |
Adds policy validation, improves unknown-policy handling in syscalls, and fixes cpuset CPU list formatting. |
src/agnocast_cie_thread_configurator/include/agnocast_cie_thread_configurator/thread_configurator_node.hpp |
Default-initializes ThreadConfig members to avoid uninitialized reads. |
src/agnocast_cie_thread_configurator/include/agnocast_cie_thread_configurator/sched_deadline.hpp |
Marks sched_setattr() as inline to prevent ODR violations across TUs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…s and log labels Signed-off-by: atsushi421 <atsushi.yano.2@tier4.jp>
Signed-off-by: atsushi421 <atsushi.yano.2@tier4.jp>
kobayu858
approved these changes
Apr 23, 2026
Coverage Report (jazzy) |
Coverage Report (humble) |
This file contains hidden or 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
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.
Description
Port code quality fixes from upstream callback_isolated_executor#51, plus additional cleanups:
inlinetosched_setattr()in header: Prevents ODR violation ifsched_deadline.hppis included from multiple translation units.ThreadConfigmembers:domain_id,priority,runtime,period, anddeadlinenow have default initializers to avoid uninitialized reads.SCHED_OTEHR) cause an immediatestd::runtime_errorat startup.elseerror path inissue_syscalls(): An unrecognized policy that somehow reachesissue_syscalls()now returnsfalseand logs an error instead of silently succeeding with only CPU affinity applied.static std::unordered_mapinstances with a single file-scopepolicy_to_sched_constmap in an anonymous namespace, using.at()for safe lookup.kPolicyToSchedConst→policy_to_sched_constto follow the project's naming convention.set_affinity_by_cgroup()now writes"0,1,2"instead of"0,1,2,"for canonical cpuset format.id=withthread=in error messages for clarity.Related links
How was this PR tested?
bash scripts/test/e2e_test_1to1.bash(required)bash scripts/test/e2e_test_2to2.bash(required)bash scripts/test/run_requires_kernel_module_tests.bash(required)Notes for reviewers
Version Update Label (Required)
Please add exactly one of the following labels to this PR:
need-major-update: User API breaking changesneed-minor-update: Internal API breaking changes (heaphook/kmod/agnocastlib compatibility)need-patch-update: Bug fixes and other changesImportant notes:
need-major-updateorneed-minor-update, please include this in the PR title as well.fix(foo)[needs major version update]: barorfeat(baz)[needs minor version update]: quxrun-build-testlabel. The PR can only be merged after the build tests pass.See CONTRIBUTING.md for detailed versioning rules.