hujun260 opened a new pull request, #18235:
URL: https://github.com/apache/nuttx/pull/18235
## Summary
Refactor the waitid() implementation with two key improvements: (1) Extract
core wait logic into a separate waittcb() helper function to reduce cyclomatic
complexity of the main function, and (2) Eliminate all goto statements and
replace them with structured control flow patterns. These changes enhance code
maintainability, improve readability, and achieve MISRA HIS standards
compliance.
## Motivation and Problem
The original waitid() implementation suffered from two code quality issues:
### Issue 1: High Cyclomatic Complexity (CCM violation)
The main waitid() function contained complex nested conditions and
comprehensive logic that resulted in high cyclomatic complexity, making the
code difficult to understand, maintain, and test.
### Issue 2: Goto Statements (HIS_GOTO violation)
The function used multiple goto statements for error handling and control
flow, which violates MISRA HIS standards. Goto statements reduce code clarity
and increase the risk of control flow errors.
### Safety Standard Compliance
- **MISRA HIS Standard**: Eliminates goto statements; reduces cyclomatic
complexity
- **Coverity Defect**: HIS_metric_violation (CCM, HIS_GOTO)
- **Pattern**: Extract helper function and use structured control flow
- **Benefit**: Enhanced code verifiability, improved maintainability, better
static analysis
## Changes Made
### Change 1: Extract waittcb() Helper Function (CCM Reduction)
1. **Function Extraction**: Create new `waittcb()` inline function
- Contains core wait logic extracted from waitid()
- Receives task context (rtcb), wait parameters, and completion status
- Returns error code for status
2. **Cyclomatic Complexity Reduction**:
- Main waitid() function complexity reduced significantly
- waittcb() contains isolated wait logic with clear responsibility
- Improved code structure and readability
3. **Parameter Reorganization**:
- waittcb() takes: rtcb, idtype, options, id, info, retains
- Clear separation of concerns between validation and wait operations
### Change 2: Eliminate Goto Statements (HIS_GOTO Elimination)
1. **Early Return Replacement**:
- Replaced `return ERROR;` calls with error flag assignment
- Early validation errors now set errcode instead of immediate return
- Unified error handling path
2. **Goto to Structured Control Flow**:
- Replaced `goto errout;` with `break;` in while loop
- Replaced `for (;;)` infinite loop with `while (errcode == OK)`
- Better loop termination semantics
3. **Conditional Block Organization**:
- Validation logic wrapped in `#ifdef CONFIG_DEBUG_FEATURES` block
- Indentation adjusted to reflect proper nesting
- Consolidated error checks removed redundant conditions
4. **Single Exit Path**:
- All code paths converge at leave_critical_section()
- Unified error handling with set_errno()
- Single return statement at function end
### File Statistics
- **File Modified**: `sched/sched/sched_waitid.c`
- **Total Lines Changed**: 545 (across both commits)
- **Commit 1 (CCM)**: 355 lines (185 insertions, 170 deletions)
- **Commit 2 (Goto)**: 190 lines (87 insertions, 103 deletions)
- **Type**: Code restructuring for quality and compliance improvement
## Impact Analysis
✅ **Backward Compatibility**: MAINTAINED - Function behavior unchanged
✅ **Code Quality**: SIGNIFICANTLY IMPROVED - Reduced complexity, eliminated
goto
✅ **MISRA Compliance**: ACHIEVED - Both CCM and HIS_GOTO violations resolved
✅ **Maintainability**: ENHANCED - Clearer code structure and responsibility
separation
✅ **Testability**: IMPROVED - Reduced complexity enables better testing
✅ **Performance**: NO IMPACT - Same logic flow, improved organization
## Verification Checklist
- [x] Compilation successful with all warning flags
- [x] waitid() behavior unchanged for valid parameters
- [x] Error conditions properly handled and reported
- [x] All goto statements eliminated and replaced
- [x] Cyclomatic complexity reduced (waittcb extracted)
- [x] Parameter validation preserved and reorganized
- [x] Child status tracking unchanged
- [x] Signal handling unchanged (SIGCHLD semantics preserved)
- [x] Critical section protection timing unchanged
- [x] Cancellation point semantics maintained
- [x] Coverity HIS_metric_violation (CCM, HIS_GOTO) defects resolved
- [x] Code review completed - all logic paths tested
## Testing Scenarios
1. **Valid Child Wait (P_PID)**: Verify waitid() waits for specific child
2. **Any Child Wait (P_ALL)**: Verify waitid() waits for any child
3. **No Children**: Verify ECHILD returned when no children exist
4. **Already Exited Child**: Verify quick return for already-exited child
5. **WNOHANG Option**: Verify non-blocking wait behavior
6. **Signal Interruption**: Verify correct handling of interrupted waits
7. **Invalid ID Types**: Verify ENOSYS returned for unsupported types
8. **Child Status Tracking**: Test with CONFIG_SCHED_CHILD_STATUS
enabled/disabled
9. **Concurrent Operations**: Test multiple threads waiting on children
10. **Error Paths**: Verify all error conditions handled correctly
## Technical Notes
### Commit 1 (CCM Reduction):
- waittcb() is inline_function to minimize call overhead
- Extracted function maintains same variable context
- Signal set creation and main loop isolated
- Child status checks and task blocking logic consolidated
### Commit 2 (Goto Elimination):
- Replaced `for (;;)` with `while (errcode == OK)` for clearer semantics
- Early error detection sets errcode flag and breaks from loop
- Removed error-specific goto labels
- Unified error handling path with single return
### Combined Result:
- Main waitid() now handles validation and error reporting
- waittcb() handles core wait semantics and signaling
- Clear separation of concerns
- No goto statements in entire implementation
- Significantly reduced cyclomatic complexity
## Related Issues
- **Category**: Code Quality & Safety Standard Compliance
- **Standard**: MISRA HIS - Cyclomatic Complexity (CCM), Goto Elimination
(HIS_GOTO)
- **Defect**: Coverity HIS_metric_violation (CCM, HIS_GOTO)
- **Subsystem**: Kernel wait process management (sched/sched/)
## Build Information
- **Compiler**: ARM GCC 10.x (primary test environment)
- **Architectures**: ARMv7-A, ARMv7-R, x86_64
- **Target**: NuttX kernel - process wait subsystem
- **Build Flags**: `-Wall -Wextra -Werror`
- **Features**: CONFIG_SCHED_WAITPID, CONFIG_SCHED_HAVE_PARENT,
CONFIG_SCHED_CHILD_STATUS configurable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
