VxWorks support #45
VxWorks support #45
Conversation
WalkthroughThe changes enhance support for the VxWorks operating system within the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
biabbas
force-pushed
the
vxworks
branch
4 times, most recently
from
4cc48f5
to
2ebc7ba
Compare
biabbas
force-pushed
the
vxworks
branch
2 times, most recently
from
bbd65de
to
1dd8d9a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/lib.rs (1)
155-157: VxWorks support added successfully, consider future refinementsThe changes successfully integrate VxWorks support into the library by treating it as a Unix-like system for thread priority management. This approach is logical and minimizes code duplication. However, as VxWorks is a real-time operating system, there might be specific considerations or optimizations that could be addressed in the future.
Consider the following for future improvements:
- Investigate if VxWorks requires any specific thread priority ranges or scheduling policies that differ from other Unix-like systems.
- Evaluate if any VxWorks-specific error handling or system calls need to be implemented.
- Test the library thoroughly on VxWorks to ensure full compatibility and optimal performance.
Also applies to: 171-173
src/unix.rs (1)
180-182: Add documentation for theSporadicscheduling policyConsider adding documentation to the
Sporadicvariant ofRealtimeThreadSchedulePolicyto explain its behavior and usage on VxWorks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (2)
- src/lib.rs (2 hunks)
- src/unix.rs (10 hunks)
🧰 Additional context used
🔇 Additional comments (11)
src/lib.rs (2)
155-157: LGTM: VxWorks support added to Unix-like systemsThe addition of
target_os = "vxworks"to the list of Unix-like target operating systems is appropriate. VxWorks, being a real-time operating system that follows POSIX standards, can likely use the same thread priority management implementation as other Unix-like systems. This change enhances the library's cross-platform support.
171-173: LGTM: Consistent VxWorks support in public exportsThe addition of
target_os = "vxworks"here is consistent with the previous change. This ensures that the Unix-like implementation is not only compiled for VxWorks targets but also publicly exported for use. This consistency is crucial for proper integration of VxWorks support in the library.src/unix.rs (9)
15-16: LGTM: Adding VxWorks support forSCHED_SPORADICThe import of
SCHED_SPORADICfor VxWorks is appropriate and correctly conditionally compiled.
56-57: LGTM: VxWorks-specificerrnoretrievalUsing
libc::errnoGet()to retrieve theerrnoon VxWorks is correct.
74-75: LGTM: VxWorks-specificerrnosettingUsing
libc::errnoSet(number)to set theerrnoon VxWorks is appropriate.
198-199: Correct mapping ofSporadicpolicy to POSIX constantMapping
RealtimeThreadSchedulePolicy::SporadictoSCHED_SPORADICis correct and ensures proper integration with VxWorks scheduling policies.
298-301: Proper handling ofSCHED_SPORADICinfrom_posixThe addition of
SCHED_SPORADICin thefrom_posixmethod allows for correct conversion from POSIX constants toThreadSchedulePolicyon VxWorks.
364-365: Include VxWorks in OS conditions for priority specificationAdding VxWorks to the list of operating systems that allow specifying the priority using
sched_paramis appropriate.
Line range hint
444-450: Ensure consistent priority mapping for VxWorksIncluding VxWorks in the conditional compilation ensures that thread priorities are correctly mapped and handled on VxWorks systems.
589-592: Extend priority-setting logic to include VxWorksIncluding VxWorks in the condition for setting thread priority via
pthread_setschedparamensures that priorities are correctly set on VxWorks.
631-631: Appropriate exclusion ofsetpriorityfor VxWorksExcluding the call to
libc::setpriorityfor VxWorks is appropriate since VxWorks handles thread priorities differently, andsetprioritymay not be applicable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/unix.rs (1)
631-633: Clarify comments regardingsetpriorityon VxWorksThe addition of
#[cfg(not(target_os = "vxworks"))]correctly excludes the call tolibc::setpriority()on VxWorks since it is unavailable. The comments can be improved for clarity.Consider updating the comments as follows:
- // VxWorks does not have setpriority function call. Control never - // reaches this statement because of the cfg condition in if + // VxWorks does not have a `setpriority` function call. + // This call is excluded on VxWorks due to the `cfg` condition above.src/vxworks.rs (2)
1-5: Update Module Documentation to Reflect VxWorks SupportThe module documentation indicates that it defines Unix thread control, but this module is specifically for VxWorks. Please update the documentation to accurately reflect the purpose of this module.
Apply this diff to correct the documentation:
-//! This module defines the unix thread control. +//! This module defines the VxWorks thread control.
147-151: Rename Variable_Sched_paramtosched_paramfor ClarityIn the function
from_posix, the parameter_Sched_paramis used within the function. Prefixing variable names with an underscore in Rust typically indicates that the variable is unused. For clarity and to adhere to Rust naming conventions, consider renaming_Sched_paramtosched_param.Apply this diff to rename the variable:
-fn from_posix(_Sched_param: libc::_Sched_param) -> Self { +fn from_posix(sched_param: libc::_Sched_param) -> Self { ScheduleParams { - sched_priority: _Sched_param.sched_priority, + sched_priority: sched_param.sched_priority, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/unix.rs (10 hunks)
- src/vxworks.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (10)
src/unix.rs (9)
15-16: AddSCHED_SPORADICimport for VxWorksThe conditional import of
SCHED_SPORADICunder#[cfg(target_os = "vxworks")]correctly includes the necessary constant for VxWorks support.
56-57: Implementerrno()function for VxWorksUsing
libc::errnoGet()in theerrno()function appropriately retrieves the error number on VxWorks systems.
74-75: Implementset_errno()function for VxWorksUtilizing
libc::errnoSet(number)in theset_errno()function correctly sets the error number on VxWorks.
180-182: AddSporadicscheduling policy supportIntroducing the
Sporadicvariant inRealtimeThreadSchedulePolicyadds support for theSCHED_SPORADICpolicy on VxWorks systems.
198-199: MapSporadicpolicy into_posix()Mapping
RealtimeThreadSchedulePolicy::SporadictoSCHED_SPORADICin theto_posix()method ensures correct POSIX value translation for VxWorks.
298-301: HandleSCHED_SPORADICinfrom_posix()Adding
SCHED_SPORADIChandling in thefrom_posix()function accurately translates the POSIX policy toRealtimeThreadSchedulePolicy::Sporadicfor VxWorks.
364-365: Support priority specification on VxWorksIncluding
target_os = "vxworks"in the condition allows specifying thread priority using scheduling parameters on VxWorks, similar to macOS and iOS.
Line range hint
444-448: Extend priority mapping for VxWorks into_posix()Adding
target_os = "vxworks"to the condition into_posix()enables correct mapping ofThreadPriorityvalues to POSIX priorities on VxWorks, consistent with macOS and iOS behavior.
589-592: Enable priority setting on VxWorks inset_thread_priority_and_policy()Including VxWorks in the condition allows setting thread priority directly using
pthread_setschedparam()on VxWorks, aligning with the implementation on macOS and iOS.src/vxworks.rs (1)
18-21: 🛠️ Refactor suggestionPrefer Using
libcDefinitions for Scheduling ConstantsThe constants
SCHED_FIFO,SCHED_RR,SCHED_OTHER, andSCHED_SPORADICare redefined with hardcoded values. To improve maintainability and ensure correctness, consider using the definitions from thelibccrate if they are available for VxWorks.Run the following script to verify if these constants are defined in
libcfor VxWorks:If they are defined, you can replace the hardcoded constants with:
use libc::{SCHED_FIFO, SCHED_RR, SCHED_OTHER, SCHED_SPORADIC};
|
r? @iddm |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/unix.rs (1)
Line range hint
590-643: Simplify VxWorks-specific handling in set_thread_priority_and_policyThe addition of VxWorks to the list of operating systems that can set priority using
pthread_setschedparamis correct. However, the newcfg_ifblock for VxWorks-specific handling ofsetpriority(lines 633-643) seems unnecessary and potentially problematic:
- It's inside a block that should only be executed for normal priority threads.
- The comment suggests this code is never reached for VxWorks.
- It returns an error instead of setting the priority.
Consider removing the VxWorks-specific
cfg_ifblock and simplifying the code as follows:if matches!(policy, ThreadSchedulePolicy::Realtime(_)) - || cfg!(any(target_os = "macos", target_os = "ios", target_os = "vxworks")) + || cfg!(any(target_os = "macos", target_os = "ios")) { // If the policy is a realtime one, the priority is set via // pthread_setschedparam. let params = ScheduleParams { sched_priority: fixed_priority, } .into_posix(); let ret = unsafe { libc::pthread_setschedparam( native, policy.to_posix(), ¶ms as *const libc::sched_param, ) }; match ret { 0 => Ok(()), e => Err(Error::OS(e)), } } else { // Normal priority threads must be set with static priority 0. let params = ScheduleParams { sched_priority: 0 }.into_posix(); let ret = unsafe { libc::pthread_setschedparam( native, policy.to_posix(), ¶ms as *const libc::sched_param, ) }; if ret != 0 { return Err(Error::OS(ret)); } // Normal priority threads adjust relative priority through niceness. set_errno(0); - // VxWorks does not have setpriority function call. Control never - // reaches this statement because of the cfg condition in if - cfg_if::cfg_if!{ - if #[cfg(target_os = "vxworks")] { - return Err(Error::Priority("Could not set priority as set priority is not available on vxworks")); - } else { - let ret = unsafe { libc::setpriority(libc::PRIO_PROCESS, 0, fixed_priority) }; - if ret != 0 { - return Err(Error::OS(errno())); - } - } - } + let ret = unsafe { libc::setpriority(libc::PRIO_PROCESS, 0, fixed_priority) }; + if ret != 0 { + return Err(Error::OS(errno())); + } Ok(()) }This simplification removes the unnecessary VxWorks-specific handling and maintains the correct behavior for other operating systems.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/unix.rs (10 hunks)
🧰 Additional context used
🔇 Additional comments (6)
src/unix.rs (6)
15-16: LGTM: Addition of SCHED_SPORADIC for VxWorksThe addition of
SCHED_SPORADICfor VxWorks is correctly implemented and aligns with the PR objective of adding VxWorks support.
56-57: LGTM: VxWorks-specific error handling addedThe addition of VxWorks-specific error handling using
libc::errnoGet()is correctly implemented and consistent with error handling for other operating systems.
74-75: LGTM: VxWorks-specific error setting addedThe addition of VxWorks-specific error setting using
libc::errnoSet(number)is correctly implemented and consistent with error setting for other operating systems.
180-183: LGTM: Sporadic variant added to RealtimeThreadSchedulePolicyThe addition of the Sporadic variant to the RealtimeThreadSchedulePolicy enum is well-implemented and documented. It correctly supports VxWorks-specific functionality.
199-200: LGTM: Sporadic case added to to_posix methodThe addition of the Sporadic case in the RealtimeThreadSchedulePolicy::to_posix method is correct and consistent with the earlier addition of the Sporadic variant.
445-445: LGTM: VxWorks added to normal thread scheduling cfg attributeThe addition of VxWorks to the cfg attribute for normal thread scheduling is correct and consistent with the overall goal of adding VxWorks support.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/unix.rs (1)
Line range hint
590-623: LGTM with suggestion: VxWorks support added to set_thread_priority_and_policyThe changes to include VxWorks in the priority setting logic are correct. However, there's a potential improvement for the VxWorks-specific
setpriorityfunction:
- The current implementation panics, which might not be the best approach for a library function.
- Consider returning an error instead of panicking.
Consider modifying the VxWorks-specific
setpriorityfunction to return an error instead of panicking:#[cfg(target_os = "vxworks")] unsafe fn setpriority(_which: u32, _who: u32, _priority: libc::c_int) -> libc::c_int { libc::ENOSYS // Or another appropriate error code }This change would allow the caller to handle the unsupported function more gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/unix.rs (11 hunks)
🧰 Additional context used
📓 Learnings (1)
src/unix.rs (2)
Learnt from: biabbas PR: iddm/thread-priority#45 File: src/unix.rs:445-445 Timestamp: 2024-10-22T09:16:09.579Z Learning: Consistently including `target_os = "vxworks"` in `cfg` attributes is important to ensure full VxWorks support across the codebase.Learnt from: biabbas PR: iddm/thread-priority#45 File: src/unix.rs:445-445 Timestamp: 2024-10-22T03:27:30.771Z Learning: VxWorks falls back to Unix code for most functionality, so missing `target_os = "vxworks"` in `cfg` attributes may not be an issue.
🔇 Additional comments (7)
src/unix.rs (7)
15-16: LGTM: VxWorks-specific scheduling policy addedThe addition of
SCHED_SPORADICfor VxWorks is correctly implemented with the appropriate conditional compilation directive.
56-57: LGTM: VxWorks-specific error handling addedThe addition of VxWorks-specific error handling using
libc::errnoGet()is correctly implemented with the appropriate conditional compilation directive.
74-75: LGTM: VxWorks-specific error setting addedThe addition of VxWorks-specific error setting using
libc::errnoSet(number)is correctly implemented with the appropriate conditional compilation directive.
180-183: LGTM: Sporadic scheduling policy added for VxWorksThe addition of the
Sporadicvariant to theRealtimeThreadSchedulePolicyenum is correctly implemented with the appropriate conditional compilation directive and clear documentation.
199-200: LGTM: Sporadic policy mapping addedThe addition of the
Sporadiccase in theRealtimeThreadSchedulePolicy::to_posixmethod is correctly implemented with the appropriate conditional compilation directive.
299-302: LGTM: Sporadic policy parsing addedThe addition of the
Sporadiccase in theThreadSchedulePolicy::from_posixmethod is correctly implemented with the appropriate conditional compilation directive.
Line range hint
1-923: Summary: VxWorks support successfully implementedThe changes to add VxWorks support in
src/unix.rsare well-implemented and consistent. Key points:
- Appropriate use of conditional compilation for VxWorks-specific code.
- Addition of the Sporadic scheduling policy for VxWorks.
- VxWorks-specific error handling and setting.
- Modifications to existing functions to accommodate VxWorks.
The only suggestion for improvement is to modify the VxWorks-specific
setpriorityfunction to return an error instead of panicking.Overall, the implementation successfully adds VxWorks support to the thread priority management functionality.
|
Cargo test fails for VxWorks. The max value check in lib.rs for threadpriority seems to be the problem. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/lib.rs (2)
155-155: Ensure Consistency in Conditional Compilation StatementsIn both conditional compilation blocks,
target_os = "vxworks"has been added. However, the entry fortarget_os = "netbsd"has been moved. Ensure thatnetbsdis included consistently in both blocks to maintain support across all intended Unix-like platforms.Also applies to: 171-171
248-253: Update Documentation forThreadPriorityValue::MAXThe documentation for
ThreadPriorityValue::MAXshould mention that the maximum value differs on VxWorks. This will help users understand the platform-specific behavior.Consider adding a note to the documentation:
/// The maximum value for a thread priority. /// /// - On VxWorks, the maximum value is `255`. /// - On other platforms, the maximum value is `99`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/lib.rs (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
src/lib.rs (2)
248-253:⚠️ Potential issueUpdate Error Message in
try_fromImplementationThe
try_frommethod's error message still references the old priority range[0;99]. Since the maximum priority value is now255for VxWorks, the error message should be updated to reflect the new valid range.Apply the following diff to correct the error message:
fn try_from(value: u8) -> Result<Self, Self::Error> { if (Self::MIN..=Self::MAX).contains(&value) { Ok(Self(value)) } else { - Err("The value is not in the range of [0;99]") + Err("The value is not in the valid priority range") } }Likely invalid or redundant comment.
248-253: Verify Usage ofThreadPriorityValue::MAXin the CodebaseThe change in
MAXvalue for VxWorks may affect other parts of the code that rely on this constant. Verify that all usages ofThreadPriorityValue::MAXproperly handle the new maximum value for VxWorks.Run the following script to find all instances where
ThreadPriorityValue::MAXis used:✅ Verification successful
Usage of
ThreadPriorityValue::MAXVerifiedAll usages of
ThreadPriorityValue::MAXproperly handle the new maximum value for VxWorks.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of ThreadPriorityValue::MAX in the codebase # Search for ThreadPriorityValue::MAX and display the surrounding 3 lines for context rg -A 3 -B 3 'ThreadPriorityValue::MAX'Length of output: 676
biabbas
force-pushed
the
vxworks
branch
3 times, most recently
from
6f599a6
to
df5ff42
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/unix.rs (2)
180-183: LGTM: VxWorks-specificSporadicvariant added correctlyThe addition of the
Sporadicvariant toRealtimeThreadSchedulePolicyenum is correctly implemented with the appropriatecfgattribute for VxWorks. The comment provides a brief description of the variant.Consider expanding the comment to provide more context about when and why this scheduling policy might be used in VxWorks systems.
Line range hint
590-623: VxWorks support added, but error handling could be improvedThe changes correctly add VxWorks to the list of OSes where priority can be set using
pthread_setschedparam. The VxWorks-specific handling ofsetpriorityis also added.However, the current implementation panics if
setpriorityis called on VxWorks. Consider returning an error instead of panicking for better error handling in production code. For example:#[cfg(target_os = "vxworks")] unsafe fn setpriority(_which: u32, _who: u32, _priority: libc::c_int) -> libc::c_int { -1 // Return an error code } // Then, in the calling code: if cfg!(target_os = "vxworks") { return Err(Error::Priority("Set priority function not supported by VxWorks")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/lib.rs (4 hunks)
- src/unix.rs (11 hunks)
- tests/unix.rs (2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/unix.rs (2)
Learnt from: biabbas PR: iddm/thread-priority#45 File: src/unix.rs:445-445 Timestamp: 2024-10-22T09:16:09.579Z Learning: Consistently including `target_os = "vxworks"` in `cfg` attributes is important to ensure full VxWorks support across the codebase.Learnt from: biabbas PR: iddm/thread-priority#45 File: src/unix.rs:445-445 Timestamp: 2024-10-22T03:27:30.771Z Learning: VxWorks falls back to Unix code for most functionality, so missing `target_os = "vxworks"` in `cfg` attributes may not be an issue.
🔇 Additional comments (8)
src/unix.rs (4)
15-16: LGTM: VxWorks-specific import added correctlyThe addition of
SCHED_SPORADICimport for VxWorks is correctly implemented with the appropriatecfgattribute. This change enhances VxWorks support in the codebase.
199-200: LGTM: VxWorks-specificSporadicpolicy handling added correctlyThe addition of the
Sporadiccase in theto_posixmethod is correctly implemented with the appropriatecfgattribute for VxWorks. This change ensures proper handling of the new scheduling policy for VxWorks systems.
365-366: LGTM: VxWorks support added to priority edge value calculationThe addition of VxWorks to the list of OSes in the
ThreadPriority::get_edge_value_for_policymethod is correct and consistent with the other VxWorks-related changes in the file.
Line range hint
1-853: Overall assessment: VxWorks support successfully implementedThe changes in this file successfully implement VxWorks support with appropriate use of conditional compilation and handling of VxWorks-specific behaviors. The modifications include:
- Adding VxWorks-specific imports
- Extending the
RealtimeThreadSchedulePolicyenum with aSporadicvariant for VxWorks- Updating the
to_posixmethod to handle the newSporadicpolicy- Modifying the
set_thread_priority_and_policyfunction to accommodate VxWorks- Including VxWorks in priority edge value calculations
While the implementation is generally solid, consider improving the error handling for the
setpriorityfunction on VxWorks as suggested earlier.tests/unix.rs (2)
Line range hint
49-54: Verify VxWorks compatibility inget_and_set_priority_with_normal_policiesThe addition of
target_os = "vxworks"allows this test to run on VxWorks. However, according to the PR objectives, tests are failing on VxWorks due to thread priority range issues. VxWorks may have different priority ranges for scheduling policies compared to other Unix-like systems. Please review and adjust the test cases to accommodate VxWorks-specific priority constraints to ensure they pass successfully.Would you like assistance in modifying the test cases for VxWorks-specific behavior?
Line range hint
111-118: Ensureget_and_set_priority_with_normal_policyaligns with VxWorks behaviorBy including
target_os = "vxworks"in the conditional compilation, this test now runs on VxWorks. It's important to confirm that the assumptions in this test hold true for VxWorks, especially regarding the acceptance of non-zero priority values with theSCHED_OTHERpolicy. Since VxWorks may handle thread priorities differently, adjustments to the test or handling logic may be necessary.Would you like assistance in adapting the test to VxWorks-specific scheduling policies?
src/lib.rs (2)
155-158: Approved: Added VxWorks to conditional compilation directivesThe inclusion of
target_os = "vxworks"in the conditional compilation directives correctly adds support for VxWorks. This ensures that the Unix module is compiled for VxWorks systems.
171-174: Approved: Exporting Unix module for VxWorksThe update to
pub use unix::*;now includes VxWorks, allowing the Unix-specific implementations to be used on VxWorks platforms.
biabbas
force-pushed
the
vxworks
branch
2 times, most recently
from
d7a7769
to
eb541fb
Compare
|
Test results for VxWorks |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/unix.rs (1)
180-183: Enhance documentation for VxWorks Sporadic scheduling
While the basic documentation is present, it would be helpful to add more details about:
- How Sporadic scheduling behaves in VxWorks
- When to use this policy over Fifo or RoundRobin
- Any VxWorks-specific limitations or considerations
Apply this diff to improve the documentation:
- /// A sporadic scheduling policy specific to VxWorks.
+ /// A sporadic scheduling policy specific to VxWorks.
+ ///
+ /// Sporadic threads are similar to FIFO threads but with additional
+ /// constraints on execution time and period. They are useful for
+ /// tasks that need precise control over CPU utilization while
+ /// maintaining real-time characteristics.Also applies to: 199-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/lib.rs (4 hunks)
- src/unix.rs (11 hunks)
- tests/unix.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unix.rs
🧰 Additional context used
📓 Learnings (1)
src/unix.rs (2)
Learnt from: biabbas
PR: iddm/thread-priority#45
File: src/unix.rs:445-445
Timestamp: 2024-10-22T09:16:09.579Z
Learning: Consistently including `target_os = "vxworks"` in `cfg` attributes is important to ensure full VxWorks support across the codebase.
Learnt from: biabbas
PR: iddm/thread-priority#45
File: src/unix.rs:445-445
Timestamp: 2024-10-22T03:27:30.771Z
Learning: VxWorks falls back to Unix code for most functionality, so missing `target_os = "vxworks"` in `cfg` attributes may not be an issue.
🔇 Additional comments (6)
src/lib.rs (4)
155-155: LGTM: VxWorks support added consistently
The VxWorks operating system has been correctly added to the conditional compilation directives for Unix-like systems.
Also applies to: 171-171
255-265: LGTM: Improved error handling with dynamic range values
The error handling has been enhanced to:
- Use String instead of &'static str for dynamic error messages
- Include the actual valid range in the error message, which is crucial for VxWorks support
Line range hint 817-829: LGTM: ThreadScopeExt implementation is correct
The implementation follows Rust's standard patterns for scoped threads and properly handles priority setting.
249-249: Verify the impact of the new MAX value
The MAX value has been updated to support VxWorks' wider priority range (0-255). Let's verify that this change doesn't affect existing code that might assume the previous maximum of 99.
✅ Verification successful
Let's examine the actual usage of these priority values and tests to make a proper assessment.
Let's check the actual implementation and tests to see how the priority values are validated and used.
Based on the comprehensive analysis of the code, I can now provide the final response:
The MAX value change is safely implemented with proper validation
The change to support VxWorks' priority range (0-255) is well-handled because:
- The
try_fromimplementation properly validates values against the new conditional MAX - Windows implementation explicitly handles the range 0-99 in its priority mapping
- Unix implementation safely maps priorities using ratio calculations that work with both ranges
- No hardcoded assumptions about the maximum value were found in tests or implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of ThreadPriorityValue::MAX or hardcoded values
rg -l "ThreadPriorityValue::MAX|priority.*99|priority.*255"
# Search for potential test cases that might need updating
rg -l "test.*priority.*range|assert.*priority"
Length of output: 201
Script:
#!/bin/bash
# Check actual usage in unix.rs
rg -A 5 -B 5 "ThreadPriorityValue::MAX|priority.*99|priority.*255" src/unix.rs
# Check test assertions and priority range tests
rg -A 5 -B 5 "test.*priority.*range|assert.*priority" tests/unix.rs tests/windows.rs
# Check documentation mentions
rg -A 5 -B 5 "priority.*range|ThreadPriorityValue::MAX" README.md
Length of output: 1921
Script:
#!/bin/bash
# Check priority value validation and usage in lib.rs
rg -A 10 -B 10 "impl ThreadPriorityValue|impl ThreadPriority" src/lib.rs
# Check for any range checks or validations
ast-grep --pattern 'fn try_from($$$) -> Result<$$$> { $$$ }'
# Check for any priority conversion or mapping logic
rg -A 10 -B 10 "priority.*into|from.*priority" src/lib.rs src/unix.rs src/windows.rs
Length of output: 21107
src/unix.rs (2)
13-14: LGTM: VxWorks error handling implementation
The implementation correctly uses VxWorks-specific error handling functions errnoGet() and errnoSet().
Also applies to: 56-57, 74-75
445-452: Verify thread priority ranges for VxWorks
Based on the PR objectives mentioning test failures with priority range [0;99], we should verify that the priority ranges are correctly handled for VxWorks.
Also applies to: 590-597
✅ Verification successful
Priority range handling for VxWorks is correctly implemented
The verification shows that VxWorks priority ranges are properly handled:
- VxWorks has a wider priority range [0..255] compared to other platforms [0..99], as defined in
ThreadPriorityValue::MAX - Priority validation is handled through
to_allowed_value_for_policywhich usesget_min_priorityandget_max_priorityto enforce platform-specific ranges - VxWorks-specific scheduling policies (including SCHED_SPORADIC) are properly configured with appropriate cfg attributes
- Priority setting is implemented via pthread_setschedparam for VxWorks, similar to macOS/iOS
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check priority range handling in tests and implementation
# Search for priority range checks
echo "Priority range checks:"
rg -A 5 "PriorityNotInRange|allowed_range|min_priority|max_priority" src/
# Search for VxWorks-specific test cases
echo -e "\nVxWorks test cases:"
rg -A 5 "target_os.*vxworks.*test" src/ tests/
Length of output: 2931
Script:
#!/bin/bash
# Let's check for VxWorks-specific priority handling and policy implementations
# Search for VxWorks-specific cfg attributes and their associated code
echo "VxWorks-specific priority handling:"
rg -B 2 -A 5 'target_os.*vxworks' src/
# Search for policy-to-priority value conversions
echo -e "\nPolicy to priority conversions:"
ast-grep --pattern 'fn to_allowed_value_for_policy($$$) { $$$ }'
# Check for any VxWorks-specific constants or priority ranges
echo -e "\nPriority range constants:"
rg -A 2 'const.*PRIORITY|PRIORITY_.*=' src/
Length of output: 8783
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
tests/unix.rs (1)
Line range hint
142-220: Fix test assumptions about priority ranges for VxWorks.The test
get_and_set_priority_with_realtime_policy_requires_capabilitiesis failing for VxWorks because it assumes a priority range of [0;99] which is incorrect for VxWorks (which uses [0;255]). This explains the test failures mentioned in the PR objectives forcase_1_fifoandcase_2_roundrobin.The test should be updated to handle OS-specific priority ranges. Here's a suggested fix:
#[rstest] #[case::fifo(ThreadSchedulePolicy::Realtime(RealtimeThreadSchedulePolicy::Fifo))] #[case::roundrobin(ThreadSchedulePolicy::Realtime(RealtimeThreadSchedulePolicy::RoundRobin))] fn get_and_set_priority_with_realtime_policy_requires_capabilities( #[case] realtime_policy: ThreadSchedulePolicy, ) { let thread_id = thread_native_id(); + // Get the OS-specific max/min values instead of assuming [0;99] let max_value = ThreadPriority::max_value_for_policy(realtime_policy).unwrap(); let min_value = ThreadPriority::min_value_for_policy(realtime_policy).unwrap(); assert_eq!( set_thread_priority_and_policy(thread_id, ThreadPriority::Max, realtime_policy,), Ok(()) );This change will ensure the test uses the correct priority ranges for each OS, fixing the VxWorks test failures.
src/unix.rs (1)
180-183: Enhance documentation for Sporadic scheduling policyWhile the implementation is correct, the documentation could be more detailed to help users understand:
- When to use Sporadic scheduling over other policies
- Any VxWorks-specific considerations or limitations
Consider expanding the documentation:
- /// A sporadic scheduling policy specific to VxWorks. + /// A sporadic scheduling policy specific to VxWorks that allows threads to execute with + /// real-time priority for a specified amount of time within a given period. + /// This policy is useful for tasks that need to meet soft real-time constraints while + /// efficiently sharing CPU resources.Also applies to: 199-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/lib.rs(4 hunks)src/unix.rs(11 hunks)tests/unix.rs(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/unix.rs (2)
Learnt from: biabbas
PR: iddm/thread-priority#45
File: src/unix.rs:445-445
Timestamp: 2024-10-22T09:16:09.579Z
Learning: Consistently including `target_os = "vxworks"` in `cfg` attributes is important to ensure full VxWorks support across the codebase.
Learnt from: biabbas
PR: iddm/thread-priority#45
File: src/unix.rs:445-445
Timestamp: 2024-10-22T03:27:30.771Z
Learning: VxWorks falls back to Unix code for most functionality, so missing `target_os = "vxworks"` in `cfg` attributes may not be an issue.
🔇 Additional comments (7)
src/lib.rs (3)
155-155: LGTM: VxWorks configuration is properly integrated
The addition of VxWorks to the Unix-like systems configuration is correct and consistent.
Also applies to: 171-171
249-249: LGTM: Thread priority range correctly adjusted for VxWorks
The MAX constant is properly configured to support VxWorks' wider priority range (0-255) while maintaining the existing range (0-99) for other platforms.
255-265: LGTM: Error handling improved with dynamic range values
The error handling has been enhanced to:
- Use String type for dynamic error messages
- Include actual MIN and MAX values in the error message
- Provide clearer feedback about valid priority ranges
src/unix.rs (4)
13-14: LGTM: VxWorks error handling implementation
The implementation correctly uses VxWorks-specific errnoGet and errnoSet functions for error handling, maintaining consistency with other OS implementations.
Also applies to: 56-57, 74-75
445-452: LGTM: VxWorks priority handling implementation
The implementation correctly handles VxWorks thread priorities:
- Uses pthread_setschedparam for priority setting, similar to macOS/iOS
- Properly handles priority conversion for VxWorks threads
Also applies to: 590-597
619-632: LGTM: VxWorks setpriority implementation
The implementation correctly handles the absence of setpriority in VxWorks:
- Returns ENOSYS to indicate unsupported functionality
- Sets errno correctly
- Uses proper cfg attributes for conditional compilation
445-452: Verify thread priority ranges for VxWorks
Let's verify that the thread priority ranges are correctly handled for VxWorks.
✅ Verification successful
The search results show clear evidence of VxWorks-specific thread priority handling. In src/lib.rs, there's a dedicated constant MAX that's set to 255 for VxWorks (compared to 99 for other platforms):
pub const MAX: u8 = if cfg!(target_os = "vxworks") { 255 } else { 99 };Let me verify the complete implementation by checking how these values are used in VxWorks-specific code paths.
Thread priority ranges for VxWorks are correctly implemented
The verification confirms proper handling of VxWorks thread priorities:
- VxWorks-specific maximum priority is set to 255 (compared to 99 for other platforms)
- VxWorks supports the SCHED_SPORADIC scheduling policy
- Priority handling for VxWorks follows the same pattern as macOS/iOS for SCHED_OTHER policy
- The implementation correctly maps priority values through
to_allowed_value_for_policy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check thread priority range handling for VxWorks
# Test: Search for VxWorks-specific priority range definitions
rg -A 5 'ThreadPriorityValue|MAX_PRIORITY' src/
Length of output: 4826
Script:
#!/bin/bash
# Search for VxWorks-specific scheduling code
rg -B 3 -A 3 'target_os = "vxworks"' src/
Length of output: 5881
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/unix.rs(3 hunks)
🧰 Additional context used
📓 Learnings (1)
tests/unix.rs (2)
Learnt from: biabbas
PR: iddm/thread-priority#45
File: src/unix.rs:445-445
Timestamp: 2024-10-22T09:16:09.579Z
Learning: Consistently including `target_os = "vxworks"` in `cfg` attributes is important to ensure full VxWorks support across the codebase.
Learnt from: biabbas
PR: iddm/thread-priority#45
File: src/unix.rs:445-445
Timestamp: 2024-10-22T03:27:30.771Z
Learning: VxWorks falls back to Unix code for most functionality, so missing `target_os = "vxworks"` in `cfg` attributes may not be an issue.
🔇 Additional comments (3)
tests/unix.rs (3)
49-49: LGTM! VxWorks correctly grouped with Unix-like systems.
The addition of VxWorks to the OS target list is appropriate as it shares similar thread priority behavior with other Unix-like systems.
74-81: LGTM! Comprehensive test coverage for VxWorks scheduling policies.
The test cases correctly:
- Use VxWorks priority range [0..=255] consistently across all policies
- Include all scheduling policies including the VxWorks-specific Sporadic policy
68-73: Verify priority range exclusions for VxWorks.
The exclusion of VxWorks from standard Unix priority ranges ([-20..=19] for Other and [0..=99] for Realtime) is correct, as VxWorks uses a different priority range.
Let's verify the priority ranges in the codebase:
✅ Verification successful
Let me gather more information about the priority ranges, particularly focusing on VxWorks specific code.
Based on the comprehensive search results, I can now verify the priority ranges for VxWorks and generate the final response:
Priority range exclusions for VxWorks are correctly configured
The code correctly handles VxWorks-specific priority ranges:
- VxWorks uses [0..=255] range for all policies (Other, Fifo, RoundRobin, Sporadic)
- Other Unix systems use:
- [-20..=19] for Normal/Other policy
- [0..=99] for Realtime policies (Fifo, RoundRobin)
This is verified by:
- ThreadPriorityValue::MAX constant in src/lib.rs:
if cfg!(target_os = "vxworks") { 255 } else { 99 } - Test cases in tests/unix.rs with correct ranges for each OS
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify priority range definitions for VxWorks
# Look for priority range definitions and validations
rg -B2 -A2 '0..=\d+' src/
Length of output: 270
Script:
#!/bin/bash
# Search for VxWorks specific priority handling
rg -B3 -A3 "vxworks|VxWorks" --type rust
# Search for priority range validations
rg -B3 -A3 "priority|Priority" --type rust
Length of output: 98789
Update src/unix.rs for vxWorks tests Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/unix.rs (3)
180-183: Enhance documentation for VxWorks SCHED_SPORADIC policy.While the implementation is correct, consider adding more detailed documentation about:
- How SCHED_SPORADIC differs from FIFO/RR in VxWorks
- Any VxWorks-specific limitations or behaviors
- When to use this policy over others
Apply this diff to improve documentation:
- // Policy similar to Fifo - /// A sporadic scheduling policy specific to VxWorks. + /// A sporadic scheduling policy specific to VxWorks. + /// + /// This policy is similar to FIFO but with additional constraints: + /// - Allows specification of replenishment period + /// - Provides execution time budget control + /// - Suitable for tasks that execute periodically with sporadic behaviorAlso applies to: 199-200, 299-302
619-632: Improve setpriority implementation for VxWorks.While the implementation is functionally correct, consider these improvements:
- Extract the setpriority implementation into a separate function
- Provide a more descriptive error message through errno
Apply this diff to improve the implementation:
+#[cfg(target_os = "vxworks")] +unsafe fn setpriority_vxworks( + which: u32, + who: u32, + priority: libc::c_int, +) -> libc::c_int { + // VxWorks does not support the setpriority syscall + set_errno(libc::ENOSYS); + -1 +} -//VxWorks does not have set priority function -#[cfg(target_os = "vxworks")] -unsafe fn setpriority( - _which: u32, - _who: u32, - _priority: libc::c_int, -) -> libc::c_int { - set_errno(libc::ENOSYS); - -1 -} +#[cfg(target_os = "vxworks")] +use setpriority_vxworks as setpriority;
Based on the verification results, I can now provide a final response about the VxWorks support:
VxWorks support is properly implemented but has a minor inconsistency in error handling
- VxWorks has proper support for all required scheduling policies (SCHED_SPORADIC, SCHED_FIFO, SCHED_RR, SCHED_OTHER)
- The
setpriorityfunction is correctly handled with a VxWorks-specific stub that sets ENOSYS- VxWorks is properly included in the OS-specific priority handling paths alongside macOS/iOS
- However, there's an inconsistency in error handling where VxWorks uses
errnoSet()but ignores its return value withlet _, which could mask potential errorsLocations needing attention:
src/unix.rs: Theset_errnofunction should properly handle the return value fromerrnoSet()for VxWorks🔗 Analysis chain
Line range hint
1-932: Verify complete VxWorks support across the codebase.Let's verify that VxWorks support is consistently implemented across all critical paths.
Let me generate additional scripts to better verify VxWorks support.
Let me generate one final verification script with corrected regex patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify VxWorks support completeness # Test 1: Check for any missing VxWorks cfg attributes in critical paths echo "Checking for potential missing VxWorks cfg attributes..." rg --type rust "cfg\(.*target_os.*\)" | grep -v "vxworks" | grep "macos\|ios" # Test 2: Verify all required VxWorks-specific functions are implemented echo "Verifying VxWorks-specific function implementations..." rg --type rust "fn.*\(.*\).*\{" | grep -B 2 "cfg.*vxworks" # Test 3: Check for potential inconsistencies in error handling echo "Checking error handling consistency..." rg --type rust "Error::" | grep -B 2 -A 2 "cfg.*vxworks"Length of output: 746
Script:
#!/bin/bash # Description: Comprehensive VxWorks support verification # Check 1: Find all VxWorks-specific code paths echo "=== VxWorks-specific code paths ===" rg --type rust "#\[cfg\(target_os\s*=\s*\"vxworks\"\)\]" -B 2 -A 2 # Check 2: Find all VxWorks-specific types and constants echo -e "\n=== VxWorks-specific types and constants ===" rg --type rust "(?:#\[cfg\(target_os\s*=\s*\"vxworks\"\)\].*\n)?(pub\s+)?(type|const|static|struct|enum)" -A 1 # Check 3: Verify error handling paths that should include VxWorks echo -e "\n=== Error handling paths ===" rg --type rust "cfg_if! \{.*\n.*if.*target_os.*\n.*\}" -A 5 # Check 4: Find potential missing VxWorks cases in OS-specific code echo -e "\n=== Potential missing VxWorks cases ===" rg --type rust "target_os\s*=\s*\"(?!vxworks).*\".*\n.*\{" -A 3 # Check 5: Check scheduling policy implementations echo -e "\n=== Scheduling policy implementations ===" rg --type rust "(ThreadSchedulePolicy|RealtimeThreadSchedulePolicy|NormalThreadSchedulePolicy).*impl"Length of output: 4335
Script:
#!/bin/bash # Check 1: Find VxWorks-specific error handling with fixed regex echo "=== VxWorks error handling ===" rg --type rust -U "cfg_if!.*if.*vxworks.*\{" -A 5 # Check 2: Find potential missing VxWorks cases with fixed pattern echo -e "\n=== OS-specific implementations ===" rg --type rust "target_os.*=.*\"(linux|macos|ios|android|freebsd)\"" -A 2 # Check 3: Verify VxWorks scheduling policy support echo -e "\n=== VxWorks scheduling policies ===" rg --type rust "SCHED_(SPORADIC|FIFO|RR|OTHER)" -B 1 # Check 4: Find VxWorks-specific function implementations with fixed pattern echo -e "\n=== VxWorks-specific functions ===" rg --type rust "#\[cfg.*vxworks.*\](\n|\r\n).*fn.*\{" -A 2 # Check 5: Check ThreadExt implementation consistency echo -e "\n=== ThreadExt implementation ===" rg --type rust "impl.*ThreadExt.*\{" -A 10Length of output: 14743
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/lib.rs(4 hunks)src/unix.rs(11 hunks)tests/unix.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unix.rs
🧰 Additional context used
📓 Learnings (1)
src/unix.rs (2)
Learnt from: biabbas
PR: iddm/thread-priority#45
File: src/unix.rs:445-445
Timestamp: 2024-10-22T09:16:09.579Z
Learning: Consistently including `target_os = "vxworks"` in `cfg` attributes is important to ensure full VxWorks support across the codebase.
Learnt from: biabbas
PR: iddm/thread-priority#45
File: src/unix.rs:445-445
Timestamp: 2024-10-22T03:27:30.771Z
Learning: VxWorks falls back to Unix code for most functionality, so missing `target_os = "vxworks"` in `cfg` attributes may not be an issue.
🔇 Additional comments (5)
src/lib.rs (3)
155-155: LGTM: VxWorks configuration is properly integrated
The addition of VxWorks to the Unix module configuration is consistent with its Unix-like nature and follows the existing pattern for OS-specific configurations.
Also applies to: 171-171
Line range hint 817-829: LGTM: ThreadScopeExt implementation is correct
The implementation follows Rust best practices and properly handles thread priority management within scopes.
249-249: Verify test coverage for VxWorks priority range
The implementation looks good, but we should ensure proper test coverage exists for the VxWorks-specific maximum priority value (255).
Also applies to: 255-265
src/unix.rs (2)
13-14: LGTM: VxWorks error handling implementation.
The implementation correctly uses VxWorks-specific errnoGet() and errnoSet() functions, maintaining consistency with other OS implementations.
Also applies to: 56-57, 74-75
445-454: LGTM: VxWorks priority handling implementation.
The implementation correctly includes VxWorks in the list of OSes that support priority changes for normal scheduling policies, maintaining consistency with other Unix-like systems.
|
review? @iddm |
|
@iddm How do we get these changes into a stable release? |
|
Thank you! Available in https://crates.io/crates/thread-priority/1.2.0 |
Fixes #44