Migrate to our own Signal type #367
Conversation
WalkthroughThe changes in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant yash-builtin
participant yash-env
User->>yash-builtin: Send signal via kill command
yash-builtin->>yash-env: Validate and parse signal
yash-env-->>yash-builtin: Return signal number
yash-builtin->>yash-env: Execute signal handling
yash-env-->>yash-builtin: Signal action performed
yash-builtin-->>User: Confirmation of signal execution
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
This commit modifies the type of the `signal_handlings` field of `Process` from `HashMap<Signal, SignalHandling>` to `HashMap<signal::Number, SignalHandling>`. This change is necessary because the `Signal` type is being deprecated in favor of `signal::Number`. This commit also modifies the `signal_handling` and `set_signal_handling` methods of `Process` to operate on `signal::Number` instead of `Signal`.
This commit reduces the use of Signal in the virtual system by storing signal::Number in Process::caught_signals. This is a step towards removing Signal from the virtual system.
This commit changes the deliver_signal method to take a signal::Number instead of a Signal. This is a step towards removing the Signal enum from the virtual process implementation.
This commit changes the signature of Process::raise_signal to take a signal::Number instead of a Signal. This is part of the ongoing work to separate the signal names, real numbers, and virtual numbers.
Before adding a new Report type, rename the existing one to OldReport.
This commit changes the associated value of
`job::ProcessResult::{Stopped, Signaled}` from `trap::Signal` to
`signal::Number`. This change is part of the major restructuring to
remove the dependency on the `nix` crate in the virtual system.
Before adding a new Condition type, rename the existing one to a different name.
Returning a tuple of error-describing values was not ergonomic. This commit redefines the Error type as a struct with named fields. This change also makes the Error type public so that it can be used in other modules.
This commit adds two methods to `SignalSystem` to convert between signal names and numbers. These methods will be needed by `TrapSet` to operate on some specific signals such as `SIGINT` and `SIGTERM`.
This commit migrates the signal-representing type in `TrapSet` from `Signal` to `signal::Number`. All methods of `TrapSet` that previously took `Signal` now take `signal::Number`. Other functions are updated to use `signal::Name` and `signal::Number` instead of `Signal`.
This is part of the effort to replace trap::Signal with signal::Number in the System API.
This change replaces the `Signal` type with the `signal::Number` type for the `System::sigaction` method. This change is part of the ongoing work to remove the `Signal` type from the public API.
This commit changes the parameter and return types of functions that deal with signals in yash-env from trap::Signal to signal::Number. This is part of the ongoing refactoring to remove the Signal type from the public API.
This is part of the ongoing reconstruction to eliminate the use of nix types in the built-ins. This commit introduces the new signal types that are now used in the kill built-in. The `Signal` type represents a signal to be sent or printed by the kill built-in. The validity of the signal name or number is now checked at the time of execution by the system.
This type was used to represent signals in the trap module, but it has been replaced by yash_env::signal::Signal. This commit removes the old type. This commit is the final step in the extensive restructuring to remove the nix-based Signal type from the public API of the yash_env crate. Note that internal uses of the type still remain in the implementation, which we plan to address later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (3)
yash-semantics/CHANGELOG.md (1)
Line range hint
17-19: Adjust the indentation of the unordered list to match the expected level.- - `ValueState` was renamed to `Vacancy`. - - `EmptyError` was renamed to `VacantError`. - - `EmptyError::state` was renamed to `VacantError::vacancy`. + - `ValueState` was renamed to `Vacancy`. + - `EmptyError` was renamed to `VacantError`. + - `EmptyError::state` was renamed to `VacantError::vacancy`.yash-builtin/src/trap.rs (1)
Line range hint
168-181: Refactoreddisplay_trapsto utilizeCondSpec.This refactoring simplifies the display logic by leveraging the new
CondSpecstructure, which should make future modifications or extensions easier.yash-env/src/trap/state.rs (1)
Line range hint
391-613: The tests are well-structured and cover various scenarios for signal handling. However, consider adding more edge cases, especially for error conditions and signal interactions.Would you like me to help in writing additional test cases to cover more scenarios?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (42)
- yash-builtin/CHANGELOG.md (1 hunks)
- yash-builtin/src/bg.rs (14 hunks)
- yash-builtin/src/fg.rs (16 hunks)
- yash-builtin/src/jobs.rs (19 hunks)
- yash-builtin/src/kill.rs (2 hunks)
- yash-builtin/src/kill/print.rs (1 hunks)
- yash-builtin/src/kill/send.rs (6 hunks)
- yash-builtin/src/kill/signal.rs (1 hunks)
- yash-builtin/src/kill/syntax.rs (24 hunks)
- yash-builtin/src/set.rs (1 hunks)
- yash-builtin/src/trap.rs (13 hunks)
- yash-builtin/src/trap/cond.rs (1 hunks)
- yash-builtin/src/trap/syntax.rs (13 hunks)
- yash-builtin/src/wait/core.rs (6 hunks)
- yash-builtin/src/wait/status.rs (4 hunks)
- yash-cli/src/lib.rs (2 hunks)
- yash-env/CHANGELOG.md (2 hunks)
- yash-env/Cargo.toml (1 hunks)
- yash-env/src/job.rs (23 hunks)
- yash-env/src/job/fmt.rs (2 hunks)
- yash-env/src/lib.rs (11 hunks)
- yash-env/src/semantics.rs (3 hunks)
- yash-env/src/signal.rs (1 hunks)
- yash-env/src/subshell.rs (4 hunks)
- yash-env/src/system.rs (23 hunks)
- yash-env/src/system/real.rs (9 hunks)
- yash-env/src/system/real/signal.rs (1 hunks)
- yash-env/src/system/virtual.rs (25 hunks)
- yash-env/src/system/virtual/process.rs (23 hunks)
- yash-env/src/system/virtual/signal.rs (2 hunks)
- yash-env/src/trap.rs (65 hunks)
- yash-env/src/trap/cond.rs (1 hunks)
- yash-env/src/trap/state.rs (20 hunks)
- yash-semantics/CHANGELOG.md (1 hunks)
- yash-semantics/src/command.rs (3 hunks)
- yash-semantics/src/command/compound_command/subshell.rs (2 hunks)
- yash-semantics/src/command/item.rs (1 hunks)
- yash-semantics/src/command/pipeline.rs (2 hunks)
- yash-semantics/src/runner.rs (3 hunks)
- yash-semantics/src/tests.rs (2 hunks)
- yash-semantics/src/trap.rs (1 hunks)
- yash-semantics/src/trap/signal.rs (16 hunks)
Files not summarized due to errors (1)
- yash-env/src/trap.rs: Error: Message exceeds token limit
Files skipped from review due to trivial changes (2)
- yash-builtin/src/wait/status.rs
- yash-env/Cargo.toml
Additional context used
Markdownlint
yash-semantics/CHANGELOG.md
17-17: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
18-18: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
19-19: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentationyash-env/CHANGELOG.md
40-40: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
41-41: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
42-42: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
43-43: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
44-44: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
45-45: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
46-46: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
47-47: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
48-48: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
49-49: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
50-50: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
Additional comments not posted (166)
yash-builtin/CHANGELOG.md (3)
8-20: The changelog entries for version 0.2.0 are well-documented and clearly outline the additions and changes made to theyash-builtinmodule.
21-43: The changes section provides a detailed account of the modifications in function signatures and behavior, which is crucial for understanding the impact of these changes on the system.
44-47: The removal of redundant functions such askill::syntax::parse_signal_nameandkill::syntax::parse_signal_name_or_numberis a positive step towards simplifying the codebase.yash-semantics/CHANGELOG.md (1)
30-31: The update totrap::run_trap_if_caughtto useyash_env::signal::Numberinstead ofyash_env::trap::Signalaligns with the overall goal of the PR to standardize signal handling.yash-env/src/trap/cond.rs (4)
21-23: The imports are correctly updated to reflect the new dependencies and architectural changes.
32-32: The introduction ofSignal(signal::Number)in theConditionenum is a key change that supports the new signal handling strategy.
35-50: The implementation of conversions fromsignal::Numberandsignal::RawNumbertoConditionis well-executed, ensuring that signal numbers are correctly mapped to conditions.
55-77: The conversion ofConditiontosignal::RawNumberand the method to convertConditionto a string are correctly implemented, enhancing the modularity and reusability of the code.yash-env/CHANGELOG.md (1)
14-29: The addition of new methods and types related to signal handling in theSystemandSystemExinterfaces is well-documented and aligns with the PR's objectives to enhance signal management.yash-builtin/src/trap/cond.rs (6)
26-33: IntroducedCondSpecenum to abstract trap conditions.This change aligns with the PR's goal to abstract signal handling, enhancing maintainability and clarity.
35-45: Implementations ofFromforsignal::Nameandsignal::RawNumberare correctly defined.These implementations facilitate easy conversion from signal types to
CondSpec, simplifying client code.
47-59: Theresolvemethod correctly translatesCondSpectoCondition.This method ensures that the abstracted conditions can be converted back to concrete signal numbers when needed, which is crucial for integrating with existing APIs that expect
Condition.
61-75: Theto_conditionmethod provides robust handling of signal names and numbers.This method is essential for ensuring that the system can handle both named and numbered signals, which is a key part of the abstraction introduced in this PR.
77-89: Thefrom_conditionmethod allows reverse conversion fromConditiontoCondSpec.This functionality is important for scenarios where the system needs to interpret existing conditions in terms of the new abstraction.
92-99: ImplementedDisplayforCondSpecfor better debuggability and logging.Proper implementation of the
Displaytrait is crucial for logging and debugging purposes, making the system's internal state more transparent.yash-semantics/src/trap.rs (1)
80-80: Updated therun_trapfunction to use the new signal handling system.This update ensures that the
run_trapfunction aligns with the new signal handling mechanisms introduced in the PR, maintaining consistency across the codebase.yash-semantics/src/command.rs (1)
Line range hint
95-120: Updated test cases to use the new virtual signal system.The updates to the test cases ensure that they are compatible with the new signal handling system, which is crucial for maintaining the reliability of the command execution logic.
yash-cli/src/lib.rs (1)
137-141: Correctly implemented signal handling setup forSIGPIPE.This setup is crucial for ensuring that the shell behaves correctly in environments where
SIGPIPEneeds to be handled explicitly.yash-builtin/src/kill.rs (4)
156-158: The newsignalmodule is correctly imported and used.
167-169: TheSendcommand now includes asignal_originfield to track the source of the signal, enhancing traceability.
175-178: The
192-197: The execution logic for theSendandyash-builtin/src/kill/print.rs (6)
24-33: Imports and module usage have been updated to reflect the new signal handling structure. This is necessary for the new functionality introduced in the PR.
39-62: Theall_signalsfunction now correctly iterates over both non-real-time and real-time signals, providing a comprehensive listing. This change supports the enhanced signal handling capabilities introduced in the PR.
64-72: Thewrite_one_signalfunction has been adapted to handle verbose output, which is crucial for detailed debugging and logging.
74-98: TheInvalidSignalstruct provides a structured way to handle errors related to unrecognized signals, which is a robust enhancement for error handling.
100-131: The
136-140: Theexecutefunction now properly handles the results from theyash-builtin/src/wait/core.rs (5)
25-25: The import of thesignalmodule aligns with the PR's goal to enhance signal handling across the system.
39-40: TheErrorenum has been updated to include a new variant for trapped signals, improving error specificity and handling.
109-109: The use ofSIGSTOPandSIGTERMfrom thevirtualmodule is a direct consequence of the PR's changes, ensuring consistency in signal references throughout the codebase.
155-161: The handling of theSIGSTOPsignal in thewait_for_any_job_or_trapfunction is correctly implemented, reflecting the updated signal handling strategy.
Line range hint
181-198: The handling of theSIGTERMtrap in the test setup is well-implemented, demonstrating the system's ability to handle specific signals as traps.yash-env/src/job/fmt.rs (5)
24-44: The documentation and example usage of theReportstruct have been updated to reflect the new job formatting capabilities. This is crucial for clarity and usability.
49-55: The imports have been updated to include the newsignalmodule, which is necessary for the updated job state representation.
88-133: TheStateenum has been significantly refactored to usesignal::Nameinstead ofsignal::Number, aligning with the PR's objectives to improve signal representation in job reports.
135-157: Thefrom_process_statefunction now convertsProcessStateto the newStateformat using signal names, which is a critical update for consistency and clarity in job reporting.
160-193: TheReportstruct and itsDisplayimplementation have been updated to handle the new job state representation, ensuring that job reports are accurate and informative.yash-builtin/src/kill/send.rs (5)
23-23: Updated imports to reflect the new signal handling architecture.Also applies to: 32-32, 34-34
85-89: Refactoredsendfunction to use the newsignal::Numbertype instead of the previousSignaltype.
95-101: IntroducedUnsupportedSignalerror type to handle unsupported signals more explicitly.
139-157: Updatedexecutefunction to handle the newSignaltype and added error handling for unsupported signals.
176-176: Added unit tests to verify the behavior of the new signal handling logic.Also applies to: 178-178, 179-179, 182-182, 183-183
yash-builtin/src/kill/signal.rs (5)
29-36: Defined newSignalenum to represent signals by name or raw number, aligning with the new signal handling strategy.
44-57: ImplementedFromStrforSignalto parse signals from strings, supporting both names and numbers.
76-98: Implementedto_numbermethod inSignalto convert signals to their numeric representation, handling unsupported signals.
100-116: Addedto_name_and_numbermethod toSignalfor retrieving both signal name and number, useful for display and logging purposes.
127-158: Enhanced unit tests to cover new functionality inSignal, including parsing and conversion between names and numbers.Also applies to: 160-188
yash-semantics/src/command/compound_command/subshell.rs (2)
91-91: Updated imports to reflect changes in signal handling and system virtualization.Also applies to: 92-92
196-208: Enhanced job control for suspended subshells, ensuring proper state management and job list updates.yash-env/src/semantics.rs (4)
19-19: Updated imports to align with the new signal handling architecture.Also applies to: 21-21
104-114: Implemented conversion fromsignal::NumbertoExitStatus, adhering to POSIX requirements.
116-130: Implemented reverse conversion fromExitStatustosignal::Number, providing comprehensive signal handling.
257-280: Added unit tests to verify the correct conversion betweenExitStatusandsignal::Number.yash-builtin/src/trap/syntax.rs (2)
131-133: The import ofCondSpecandNamefromyash_env::signalaligns with the PR's objective to replace old signal types with new abstract types. This change supports the new signal handling strategy.
148-148: The tests have been updated to use the newCondSpecfor signal names and numbers, which is consistent with the changes in the main codebase. This ensures that the unit tests reflect the new signal handling logic.Also applies to: 160-160, 172-172, 185-187, 200-200, 209-209, 221-221, 233-233, 245-245, 257-257, 269-269, 281-281, 293-293
yash-semantics/src/tests.rs (1)
41-41: The addition ofSIGSTOPfromyash_env::system::r#virtualis consistent with the PR's goal to use more abstract signal representations. This change ensures that the tests are aligned with the new signal handling strategy.yash-semantics/src/trap/signal.rs (3)
23-23: The import of thesignalmodule is necessary due to the refactoring of signal handling in the system, replacing old signal types with new abstract types.
41-41: The functionrun_trap_if_caughtnow uses the newsignal::Numbertype, which is part of the PR's changes to unify signal handling. This is a crucial update for maintaining consistency across the codebase.
111-113: The tests have been updated to reflect the new signal handling logic, usingsignal::Numberand the newActionconfigurations. This ensures that the unit tests are aligned with the main codebase changes.
[APROVED]Also applies to: 124-124, 133-133, 156-156, 167-168, 179-179, 191-192, 208-208, 219-219, 225-225, 235-235, 245-245, 257-258, 274-274, 280-280, 296-296, 302-302
yash-semantics/src/command/item.rs (1)
284-291: The functionignore_sigttincorrectly uses the new signal handling system to set the action for theTtinsignal. This is part of the broader refactoring effort to use more abstract signal representations.yash-semantics/src/runner.rs (2)
Line range hint
308-319: Ensure proper signal handling in tests.The addition of signal handling in the test
running_traps_between_parsing_and_executingis a good practice for ensuring that the shell behaves correctly when signals are raised. This change aligns with the PR's objective of enhancing signal handling.
Line range hint
308-319: Verify the correct signal is raised in tests.Verification successful
The test function
running_traps_between_parsing_and_executingcorrectly raises theSIGUSR1signal as expected. The signal is set and then raised within the function, and the subsequent assertions check the expected behavior.
- The signal
SIGUSR1is set with an action to execute the commandecho USR1.- The signal
SIGUSR1is then raised.- The test verifies the output and exit status.
This confirms that the correct signal is raised in the test.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the correct signal is raised in the test `running_traps_between_parsing_and_executing`. # Test: Search for the test implementation. Expect: Only occurrences of the correct signal. rg --type rust $'running_traps_between_parsing_and_executing'Length of output: 144
Script:
#!/bin/bash # Extract the content of the `running_traps_between_parsing_and_executing` function from `yash-semantics/src/runner.rs`. ast-grep --lang rust --pattern $'fn running_traps_between_parsing_and_executing() { $$$ }' yash-semantics/src/runner.rsLength of output: 2063
yash-builtin/src/set.rs (1)
262-262: Ensure proper signal handling in thesetbuilt-in.The addition of
SIGTSTPhandling in thesetbuilt-in is crucial for managing job control signals appropriately. This change is consistent with the PR's goals of improving signal handling.yash-env/src/system/real/signal.rs (1)
27-399: Comprehensive update to signal handling.The extensive updates to signal handling in
yash-env/src/system/real/signal.rsare well-implemented. The use of conditional compilation to handle platform-specific signals and the introduction of real-time signal handling are in line with modern POSIX standards and enhance the robustness of the shell.yash-builtin/src/bg.rs (5)
97-97: Updated import to use the newyash_env::signalmodule.This change aligns with the PR's objective to migrate to a custom signal type, enhancing portability and control over signal handling.
165-165: Refactored signal handling to use the newsignal::Nameenum andsignal_number_from_namemethod.This change is crucial for abstracting system-dependent signal values, which is a key goal of this PR. It also adds error handling for the case where the signal number is invalid.
Also applies to: 171-173
239-239: Import statements forSIGSTOP,SIGTSTP, andSIGTTINhave been updated to reflect their new locations.This change is necessary due to the restructuring of signal definitions within the
yash_envmodule, ensuring that thebgbuilt-in uses the correct signal constants.
258-260: The testresume_job_by_index_sends_sigconthas been updated to use the new signal constants.Properly updating the test to use the new signal constants ensures that the test remains valid and that the
bgbuilt-in behaves as expected when resuming jobs.Also applies to: 281-281
311-311: Updated various test cases to use the new signal constants.These updates are necessary to ensure that the tests reflect the changes made in the main codebase and continue to test the correct behavior of the
bgbuilt-in.Also applies to: 337-337, 364-364, 419-420, 474-476, 515-515
yash-builtin/src/fg.rs (3)
97-97: Updated imports and usage of new signal types align with the PR objectives.Also applies to: 144-146, 214-214, 218-219
144-146: Proper handling of signal conversion with error checking enhances robustness.
214-214: Updated test cases to use new signal constants ensure consistency and validity.Also applies to: 218-219, 241-241, 261-261, 287-287, 317-317, 341-341, 366-366, 425-425, 434-434, 474-474, 489-489
yash-env/src/signal.rs (3)
65-166: Introduction of new signal typesNameandNumberwith comprehensive methods for handling and conversion aligns with the PR objectives.Also applies to: 168-179, 181-254, 256-263, 298-371
277-296: Introduction ofUnknownNameErrorfor robust error handling during signal name parsing enhances the system's reliability.
397-481: The implementation of theNumbertype with methods for safe and unchecked creation aligns with the need for robust and flexible signal handling.yash-builtin/src/trap.rs (6)
122-124: IntroducedCondSpecmodule and made it publicly accessible.This modularization enhances the clarity and maintainability of condition specifications in trap handling.
132-132: Added new imports to support the updated functionality.These imports are necessary for the new error handling and system interaction functionalities introduced in this file.
Also applies to: 135-135, 138-138
157-157: Updatedconditionsfield to useCondSpecinstead of previous type.This change aligns with the new abstraction for signal conditions, improving the system's flexibility and maintainability.
188-197: IntroducedErrorCauseenum to encapsulate specific error scenarios.This structured approach to error handling allows for more precise error reporting and handling within the trap functionality.
200-208: Defined a newErrorstruct to represent errors in trap execution.This new structure enhances error information encapsulation and propagation, aligning with modern Rust error handling practices.
264-275: UpdatedCommand::executeto handle new error structures andCondSpec.The modifications ensure that the command execution logic is compatible with the new error handling and condition specification mechanisms.
yash-env/src/system/virtual/signal.rs (3)
19-21: Updated imports to reflect changes in signal handling architecture.These changes are necessary to align with the new signal handling structures introduced in the system.
280-339: Implementedtry_from_raw_virtualto convert raw numbers back to signal names.This reverse mapping is essential for debugging and logging purposes, allowing easier traceability of signal-related issues.
379-419: Defined default effects for signals in the virtual system.Specifying default effects for each signal helps in managing the expected behavior of signals consistently across the virtual environment.
yash-builtin/src/jobs.rs (5)
99-99: Added import forSharedSystemto support the new signal handling mechanism.
115-115: RefactoredAccumulatorstruct to support new signal handling withshow_pidandpgid_onlyflags.Also applies to: 177-177, 186-186
127-140: UpdatedAccumulator::reportmethod to useSharedSystemfor state determination, aligning with the new signal handling architecture.
195-195: Ensured that job reporting now utilizes the new signal handling system by passingSharedSystemto thereportmethod.Also applies to: 202-202
243-243: Updated tests to reflect changes in signal handling and job state reporting.Also applies to: 264-264, 287-287, 303-303, 311-311, 337-337, 392-392, 437-437, 484-484, 521-521, 548-548, 569-569, 588-588, 608-608
yash-env/src/system/real.rs (7)
19-20: Introduced a newsignalmodule to handle signal operations, aligning with the PR's objective to improve signal handling.
348-352: Implemented new methodsvalidate_signalandsignal_number_from_nameto support the new signal handling architecture.Also applies to: 355-357
369-391: Refactoredsigactionto use the new signal handling types, improving modularity and maintainability.
Line range hint
394-408: Enhancedcaught_signalsmethod to validate and convert raw signal numbers using the new signal handling system.
419-423: Updatedkillmethod to support optional signal numbers, reflecting the new signal handling design.
499-521: Updatedwaitmethod to handle new signal types and process states, ensuring compatibility with the updated signal handling system.
685-698: Test case added to verify the correct capturing and handling of signals using the new system.yash-semantics/src/command/pipeline.rs (2)
630-635: The job control logic correctly updates the job state to stopped due toSIGSTOP.
348-348: Ensure the use ofSIGSTOPaligns with the new signal handling strategy.Verification successful
The use of
SIGSTOPaligns with the signal handling strategy across the codebase. The signal is consistently used in assertions, state settings, and signal handling mechanisms.
yash-semantics/src/command/pipeline.rsyash-semantics/src/command/compound_command/subshell.rsyash-semantics/src/tests.rsyash-builtin/src/kill.rsyash-builtin/src/trap.rsyash-builtin/src/jobs.rsyash-builtin/src/fg.rsyash-builtin/src/bg.rsyash-env/src/trap.rsyash-env/src/signal.rsyash-builtin/src/wait/status.rsyash-env/src/job.rsyash-env/src/trap/state.rsyash-builtin/src/wait/core.rsyash-env/src/system/real/signal.rsyash-env/src/system/virtual/signal.rsyash-env/src/system/virtual/process.rsyash-env/src/system/virtual.rsyash-env/src/job/fmt.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that SIGSTOP is used correctly across the system. rg --type rust $'SIGSTOP'Length of output: 9210
yash-builtin/src/kill/syntax.rs (3)
25-28: Ensure the newSignaltype andsignalmodule are correctly integrated.
345-349: Ensure theCommand::Sendstructure is correctly updated to use the new signal types.
193-195: Check the logic for handling multiple signals and ensure it aligns with the new signal handling strategy.Also applies to: 310-310
Verification successful
The
set_signalfunction and the related test cases indicate that the logic for handling multiple signals is implemented correctly. The function returns anError::MultipleSignalswhen more than one signal is specified, and the test cases validate this behavior.
- The
set_signalfunction checks ifsignal_originalready contains a field and returnsError::MultipleSignalsif it does.- The test cases
multiple_signals_error_on_option_s,multiple_signals_error_on_option_n, andmultiple_signals_error_on_bare_signal_nameconfirm that the function correctly identifies and handles multiple signals.The logic for handling multiple signals aligns with the new signal handling strategy.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of multiple signals in the system. rg --type rust $'MultipleSignals'Length of output: 622
Script:
#!/bin/bash # Description: Gather context around the usage of MultipleSignals in yash-builtin/src/kill/syntax.rs rg --context 5 'MultipleSignals' yash-builtin/src/kill/syntax.rsLength of output: 2754
yash-env/src/lib.rs (7)
230-230: Ensure proper handling of signals inwait_for_signals.This method now correctly captures and processes signals using the new
signal::Numbertype, aligning with the PR's objectives to refactor signal handling.
340-349: Validate signal handling inwait_for_subshell.The method
wait_for_subshellcorrectly integrates the new signal handling by usingsignal::Name::Chldto wait for child processes. This is a crucial part of the system's robustness in handling subprocesses.
457-457: Confirm the addition of thesignalmodule.The addition of the
signalmodule is crucial for the new signal handling architecture. This aligns with the PR's goal to refactor and improve signal handling.
472-472: Ensure correct signal handling in tests.The test
wait_for_signal_remembers_signal_in_trap_setcorrectly sets and checks signal actions, ensuring that the new signal handling is effective even in simulated environments.
539-539: Review test setup for signal handling.The setup in
poll_signals_envcorrectly initializes the environment for testing signal polling, ensuring that the new signal handling mechanisms are correctly integrated into the testing framework.
562-566: Check correctness of signal raising and polling in tests.The test
poll_signals_someeffectively demonstrates the functionality of signal polling by correctly raising and detecting a signal, which is crucial for validating the robustness of the new signal handling mechanisms.
242-242: Check for potential infinite loops inwait_for_signal.yash-env/src/subshell.rs (1)
656-657: Verify that the parent process correctly unblocks signals.This script checks if the parent process correctly restores the previous signal mask after the subshell starts. This is crucial to ensure that the parent process does not remain incorrectly blocked on these signals.
yash-env/src/system/virtual/process.rs (9)
20-20: Updated imports to use custom signal types.
82-82: ReplacedHashMap<Signal, SignalHandling>withHashMap<signal::Number, SignalHandling>to use the new signal type.
91-91: Updatedcaught_signalsto usesignal::Numberinstead ofSignal.
381-385: Implementedsignal_handlingto retrieve the handling for a given signal number, defaulting toSignalHandling::Defaultif not set.
394-397: Implementedset_signal_handlingto update the signal handling for a given signal number, returning the previous handling.
462-474: Updatedraise_signalto handle the new signal types, including proper state changes and delivery based on the signal blocking status.
602-602: Updated unit tests to use the newsignal::Numbertype and verify the behavior of signal handling methods. Ensure that all edge cases are covered in the tests.Also applies to: 638-638, 766-766, 788-788, 811-811, 827-827, 844-844, 861-861, 881-881, 907-907, 918-918
411-422: Refactoreddeliver_signalto handle the new signal types and effects. Ensure that the mapping fromsignal::NumbertoSignalEffectis correctly implemented.Verification successful
The mapping from
signal::NumbertoSignalEffectis correctly implemented in theofmethod of theSignalEffectenum. The method comprehensively handles various signal names and maps them to appropriateSignalEffectvariants.
Name::Abrtmaps toSelf::Terminate { core_dump: true }Name::Alrmmaps toSelf::Terminate { core_dump: false }Name::Busmaps toSelf::Terminate { core_dump: true }Name::ChldandName::Cldmap toSelf::NoneName::Contmaps toSelf::ResumeName::Emtmaps toSelf::Terminate { core_dump: false }Name::Fpemaps toSelf::Terminate { core_dump: true }Name::Hupmaps toSelf::Terminate { core_dump: false }Name::Illmaps toSelf::Terminate { core_dump: true }Name::Infomaps toSelf::Terminate { core_dump: false }Name::Intmaps toSelf::Terminate { core_dump: false }Name::Iomaps toSelf::Terminate { core_dump: false }Name::Iotmaps toSelf::Terminate { core_dump: true }Name::Killmaps toSelf::Terminate { core_dump: false }Name::Lostmaps toSelf::Terminate { core_dump: false }Name::Pipemaps toSelf::Terminate { core_dump: false }Name::Pollmaps toSelf::Terminate { core_dump: false }Name::Profmaps toSelf::Terminate { core_dump: false }Name::Pwrmaps toSelf::Terminate { core_dump: false }Name::Quitmaps toSelf::Terminate { core_dump: true }Name::Segvmaps toSelf::Terminate { core_dump: true }Name::Stkfltmaps toSelf::Terminate { core_dump: false }Name::Stopmaps toSelf::SuspendName::Sysmaps toSelf::Terminate { core_dump: true }Name::Termmaps toSelf::Terminate { core_dump: false }Name::Thrmaps toSelf::Terminate { core_dump: false }Name::Trapmaps toSelf::Terminate { core_dump: true }Name::Tstp,Name::Ttin, andName::Ttoumap toSelf::SuspendName::Urgmaps toSelf::NoneName::Usr1andName::Usr2map toSelf::Terminate { core_dump: false }Name::Vtalrmmaps toSelf::Terminate { core_dump: false }Name::Winchmaps toSelf::NoneName::Xcpumaps toSelf::Terminate { core_dump: true }Name::Xfszmaps toSelf::Terminate { core_dump: true }Name::Rtmin(_)andName::Rtmax(_)map toSelf::Terminate { core_dump: false }This confirms that the mapping is correctly implemented.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct mapping from `signal::Number` to `SignalEffect`. # Test: Search for usage of `SignalEffect::of`. Expect: Correct mappings. rg --type rust $'SignalEffect::of'Length of output: 147
Script:
#!/bin/bash # Locate the definition of `SignalEffect::of` and examine its implementation. ast-grep --lang rust --pattern 'impl SignalEffect { $_ fn of($_) -> $_ { $$$ } $$$ }'Length of output: 99
Script:
#!/bin/bash # Locate the definition of `SignalEffect` and then search for the `of` method within its implementation. # Search for the definition of `SignalEffect`. ast-grep --lang rust --pattern 'struct SignalEffect { $$$ }' || ast-grep --lang rust --pattern 'enum SignalEffect { $$$ }' # Search for the `of` method within the `SignalEffect` implementation. ast-grep --lang rust --pattern 'impl SignalEffect { $_ fn of($_) -> $_ { $$$ } $$$ }'Length of output: 164
Script:
#!/bin/bash # Locate the definition of `SignalEffect` and then search for the `of` method within its implementation. # Search for the definition of `SignalEffect` using a broader pattern. rg --type rust 'SignalEffect' # Search for the `of` method within the `SignalEffect` implementation using a broader pattern. rg --type rust 'fn of'Length of output: 924
Script:
#!/bin/bash # Extract and examine the `SignalEffect` enum definition and the `of` method implementation from `yash-env/src/system/virtual/signal.rs`. # Extract the `SignalEffect` enum definition. sed -n '/pub enum SignalEffect {/,/}/p' yash-env/src/system/virtual/signal.rs # Extract the `of` method implementation. sed -n '/impl SignalEffect {/,/}/p' yash-env/src/system/virtual/signal.rsLength of output: 448
Script:
#!/bin/bash # Extract the complete definitions of the `SignalEffect` enum and the `of` method from `yash-env/src/system/virtual/signal.rs`. # Extract the complete `SignalEffect` enum definition. sed -n '/pub enum SignalEffect {/,/}/p' yash-env/src/system/virtual/signal.rs # Extract the complete `of` method implementation. sed -n '/impl SignalEffect {/,/^}/p' yash-env/src/system/virtual/signal.rsLength of output: 2731
374-374: Modifieddeliver_signalto convertSignaltosignal::Numberusingfrom_signal_virtual. Ensure that all signal conversions are handled correctly across different system types.yash-env/src/trap/state.rs (1)
21-21: Ensure that theConditionandSignalSystemare correctly utilized in the context of the new signal handling architecture.Verification successful
The usage of
ConditionandSignalSystemis consistent across the codebase and aligns with the new signal handling architecture.
Conditionis used in various contexts, including signal handling and error conditions.SignalSystemis implemented and utilized correctly in theyash-envmodule.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `Condition` and `SignalSystem` across the codebase to ensure consistency. # Test: Search for the usage of `Condition` and `SignalSystem`. Expect: Consistent usage across the codebase. rg --type rust $'Condition|SignalSystem'Length of output: 19708
yash-env/src/trap.rs (23)
39-42: Refactored imports and re-exported entities to align with the new signal handling architecture.
54-61: Added methods for signal name and number conversion, enhancing the abstraction over signal handling.
164-169: Improved error handling by explicitly checking for non-trappable signals (SIGKILLandSIGSTOP) before setting actions.
239-252: Refactoredenter_subshellmethod to handle signal-specific behaviors more explicitly, improving readability and maintainability.
259-261: Ensure that signalsSIGINTandSIGQUITare ignored when specified, which is critical for correct shell behavior in subshells.
Line range hint
303-367: Significant enhancements in internal signal handling methods, allowing for more granular control over signal behaviors in different shell states.
Line range hint
413-501: The test suite has been expanded to cover the new functionalities and changes in signal handling. Ensure that all new behaviors are adequately tested.
509-523: Correct handling ofSIGKILLandSIGSTOPin tests, which should not be trappable, aligning with POSIX standards.
Line range hint
554-600: Tests for basic iteration over traps show that the state management and retrieval are functioning as expected.
Line range hint
614-631: Tests for setting traps in a subshell context are comprehensive and demonstrate the isolation between main shell and subshell environments.
Line range hint
645-684: Tests for entering subshells with different types of traps (ignore vs command) are thorough and validate the expected behavior changes.
Line range hint
694-710: The handling ofSIGCHLDin subshells is correctly implemented, ensuring that internal handlers are managed appropriately.
Line range hint
720-788: Comprehensive testing of internal handlers for various signals (SIGINT,SIGTERM,SIGQUIT, etc.) in subshell contexts.
Line range hint
798-866: Tests for stopper signals (SIGTSTP,SIGTTIN,SIGTTOU) in subshells are well-implemented, ensuring that these signals are handled correctly in interactive job-controlling shells.
Line range hint
876-910: Tests for setting traps after entering a subshell ensure that parent states are cleared as expected, which is crucial for correct signal handling in nested environments.
920-933: Tests for nested subshells demonstrate the correct clearing of parent states, ensuring isolation between shell layers.
Line range hint
942-986: Tests for ignoringSIGINTandSIGQUITwhen entering a subshell with actions set, and without actions set, are crucial for ensuring that the shell behaves correctly under different configurations.
Line range hint
992-1013: Tests for keeping stopper handlers ignored when specified, and the absence of stopper handlers, validate the shell's behavior in handling job control signals.
Line range hint
1049-1131: Tests for catching and handling signals are comprehensive, covering various scenarios and ensuring that the trap states are managed correctly.
1146-1166: Tests for enabling and disabling internal handlers cover a wide range of scenarios, ensuring robustness in the shell's signal handling capabilities.
Line range hint
1177-1230: The tests for disabling internal handlers after enabling them multiple times, and without enabling, are thorough and ensure that the shell's internal state is managed correctly across different operations.
1262-1268: Reenabling internal handlers after disabling them tests the shell's ability to restore signal handling states, which is crucial for dynamic signal management in interactive shells.
1277-1279: Testing the setting of a trap to ignore after enabling an internal handler forSIGCHLDensures that the shell can handle complex signal management scenarios.yash-env/src/job.rs (4)
45-45: Added import for the newsignalmodule.This change aligns with the PR's objective to replace
nix::signal::Signalwithsignal::Numberand other signal-related types.
132-132: UpdatedProcessResultto usesignal::Number.This change is crucial for decoupling from the
nixlibrary and using the newly defined signal types, which is a key objective of this PR.Also applies to: 136-139
190-190: MethodProcessState::stoppednow correctly usessignal::Number.This update is necessary for the internal consistency of the new signal handling system.
931-931: Updated test cases to usesignal::NumberandProcessState::stopped.These changes ensure that the test suite is aligned with the new signal handling implementation, which is essential for validating the changes made in this PR.
Also applies to: 1126-1126, 1153-1153, 1169-1169, 1178-1178, 1197-1199, 1245-1247, 1279-1280, 1318-1319, 1342-1342, 1371-1371, 1385-1386, 1402-1404, 1430-1432, 1458-1460, 1477-1477, 1488-1488, 1500-1500, 1511-1511, 1516-1516
yash-env/src/system.rs (7)
643-665: Review the implementation oftcsetpgrp_without_blockfor correct signal handling.Verification successful
The implementation of
tcsetpgrp_without_blockinyash-env/src/system.rsappears to handle signals correctly by unblockingSIGTTOU, setting the process group, and then restoring the signal mask and signal action. The usages inyash-semantics/src/command/pipeline.rsandyash-builtin/src/fg.rsalso seem to follow the correct procedure for handling foreground process group switching.
yash-semantics/src/command/pipeline.rsensures thattcsetpgrp_without_blockis called when job control is enabled and the shell is in the foreground.yash-builtin/src/fg.rsusestcsetpgrp_without_blockto put the target job in the foreground before sending theSIGCONTsignal.These usages align with the expected behavior for handling signals during process group switching.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct implementation of foreground process group switching with signal handling. # Test: Search for usages of `tcsetpgrp_without_block` to ensure it handles signals correctly. rg --type rust $'tcsetpgrp_without_block'Length of output: 556
Script:
#!/bin/bash # Extract the definition of `tcsetpgrp_without_block` from `yash-env/src/system.rs` ast-grep --lang rust --pattern 'fn tcsetpgrp_without_block(&mut self, fd: Fd, pgid: Pid) -> Result<()> { $$$ }' # Extract the usage of `tcsetpgrp_without_block` in `yash-semantics/src/command/pipeline.rs` rg --context 5 --type rust 'tcsetpgrp_without_block' yash-semantics/src/command/pipeline.rs # Extract the usage of `tcsetpgrp_without_block` in `yash-builtin/src/fg.rs` rg --context 5 --type rust 'tcsetpgrp_without_block' yash-builtin/src/fg.rsLength of output: 3458
227-234: Validate the implementation ofvalidate_signalandsignal_number_from_name.Verification successful
The implementations of
validate_signalandsignal_number_from_namehave been successfully extracted from the relevant files. Here are the details:
- In
yash-env/src/system.rs, these functions delegate their work to another implementation viaself.0.borrow().- In
yash-env/src/system/real.rsandyash-env/src/system/virtual.rs, the functions are implemented to handle real and virtual signals respectively.The implementations appear to be consistent and correctly handle the signal validation and conversion. Therefore, the review comment has been verified successfully.
validate_signalandsignal_number_from_nameare implemented correctly inyash-env/src/system.rs,yash-env/src/system/real.rs, andyash-env/src/system/virtual.rs.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct implementation of signal validation and conversion functions. # Test: Search for usages of these functions to ensure they are used correctly. rg --type rust $'validate_signal|signal_number_from_name'Length of output: 4625
Script:
#!/bin/bash # Extract the implementation of validate_signal in yash-env/src/system.rs ast-grep --lang rust --pattern 'fn validate_signal(&self, number: signal::RawNumber) -> Option<(signal::Name, signal::Number)> { $$$ }' yash-env/src/system.rs # Extract the implementation of signal_number_from_name in yash-env/src/system.rs ast-grep --lang rust --pattern 'fn signal_number_from_name(&self, name: signal::Name) -> Option<signal::Number> { $$$ }' yash-env/src/system.rs # Extract the implementation of validate_signal in yash-env/src/system/real.rs ast-grep --lang rust --pattern 'fn validate_signal(&self, number: signal::RawNumber) -> Option<(signal::Name, signal::Number)> { $$$ }' yash-env/src/system/real.rs # Extract the implementation of signal_number_from_name in yash-env/src/system/real.rs ast-grep --lang rust --pattern 'fn signal_number_from_name(&self, name: signal::Name) -> Option<signal::Number> { $$$ }' yash-env/src/system/real.rs # Extract the implementation of validate_signal in yash-env/src/system/virtual.rs ast-grep --lang rust --pattern 'fn validate_signal(&self, number: signal::RawNumber) -> Option<(signal::Name, signal::Number)> { $$$ }' yash-env/src/system/virtual.rs # Extract the implementation of signal_number_from_name in yash-env/src/system/virtual.rs ast-grep --lang rust --pattern 'fn signal_number_from_name(&self, name: signal::Name) -> Option<signal::Number> { $$$ }' yash-env/src/system/virtual.rsLength of output: 2763
1182-1190: Review thesigmaskandset_signal_handlingmethods for correct signal masking and handling.Also applies to: 1207-1207
Verification successful
The
sigmaskandset_signal_handlingmethods are implemented and used in various parts of the codebase. Thesigmaskmethod is defined inyash-env/src/system.rsand is used insubshell.rs,trap.rs, andsystem.rs. Theset_signal_handlingmethod is defined intrap.rsandsystem.rsand is used intrap.rs,system.rs, andvirtual/process.rs.The implementations of both methods appear to be consistent with their intended purposes. The
sigmaskmethod correctly handles signal masking, and theset_signal_handlingmethod correctly sets signal handling.
sigmaskmethod implementation insystem.rs:
- Correctly converts the signal number to a signal name and back to a signal number.
- Uses
SigSetto manage signal sets.- Calls the
sigmaskmethod on the system object to apply the signal mask.
set_signal_handlingmethod implementation intrap.rsandsystem.rs:
- Correctly sets the signal handling for the specified signal.
Based on the gathered information, the methods are implemented and used correctly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/binbash # Description: Verify the correct implementation of signal masking and handling. # Test: Search for usages of `sigmask` and `set_signal_handling` to ensure they are used correctly. rg --type rust $'sigmask|set_signal_handling'Length of output: 5823
Script:
#!/bin/bash # Description: Verify the implementation of the `sigmask` method and its usage in the codebase. # Check the implementation of the `sigmask` method in `system.rs`. ast-grep --lang rust --pattern 'fn sigmask(&mut self, how: SigmaskHow, signal: signal::Number) -> Result<()> { $$$ }' # Check the usage of the `sigmask` method in `subshell.rs`. rg 'sigmask' yash-env/src/subshell.rs # Check the usage of the `sigmask` method in `trap.rs`. rg 'sigmask' yash-env/src/trap.rs # Check the usage of the `sigmask` method in `system.rs`. rg 'sigmask' yash-env/src/system.rsLength of output: 2633
Script:
#!/bin/bash # Description: Verify the implementation of the `set_signal_handling` method and its usage in the codebase. # Check the implementation of the `set_signal_handling` method in `trap.rs`. ast-grep --lang rust --pattern 'fn set_signal_handling(&mut self, signal: signal::Number, handling: SignalHandling) -> Result<()> { $$$ }' # Check the implementation of the `set_signal_handling` method in `system.rs`. ast-grep --lang rust --pattern 'fn set_signal_handling(&mut self, signal: signal::Number, handling: SignalHandling) -> Result<()> { $$$ }' # Check the usage of the `set_signal_handling` method in `trap.rs`. rg 'set_signal_handling' yash-env/src/trap.rs # Check the usage of the `set_signal_handling` method in `system.rs`. rg 'set_signal_handling' yash-env/src/system.rs # Check the usage of the `set_signal_handling` method in `virtual/process.rs`. rg 'set_signal_handling' yash-env/src/system/virtual/process.rsLength of output: 2332
1515-1525: Review thewakemethod inAsyncSignalfor correct signal handling.Verification successful
The
wakemethod inAsyncSignalappears to be implemented correctly, but we need to verify its usage throughout the codebase to ensure it handles signals correctly. Thergoutput shows multiple instances ofwakebeing used, but we need to focus on the specific context of signal handling.To gather more information, let's search for the
AsyncSignalstruct and itswakemethod usage in the codebase to ensure it is correctly integrated with the signal handling logic.
Let's correct the
ast-greppattern for theimpl AsyncSignalblock and re-run the search to gather the necessary information.
The
wakemethod in theAsyncSignalstruct is implemented correctly and is designed to wake up tasks waiting for specific signals. The method iterates through theawaiters, upgrades weak references, updates the signal status, and wakes the associated waker if present.Given the context and the results from the shell scripts, the
wakemethod appears to be correctly implemented and integrated with the signal handling logic.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct implementation of signal waking in asynchronous signal handling. # Test: Search for usages of the `wake` method to ensure it handles signals correctly. rg --type rust $'wake'Length of output: 10559
Script:
#!/bin/bash # Search for the definition of the `AsyncSignal` struct and its `wake` method usage in the codebase. ast-grep --lang rust --pattern 'struct AsyncSignal { $$$ }' ast-grep --lang rust --pattern 'impl AsyncSignal { $$$ wake(&mut self, $_) { $$$ } $$$ }'Length of output: 793
Script:
#!/bin/bash # Corrected search for the implementation of the `AsyncSignal` struct and its `wake` method usage in the codebase. ast-grep --lang rust --pattern 'impl AsyncSignal { $$$ }' ast-grep --lang rust --pattern 'impl AsyncSignal { $$$ fn wake(&mut self, $_) { $$$ } $$$ }'Length of output: 3426
673-698: Review the new methodssignal_name_from_numberandsignal_number_from_exit_statusfor correctness.Verification successful
The methods
signal_name_from_numberandsignal_number_from_exit_statushave been correctly implemented and are used appropriately across multiple files. Thevalidate_signalmethod, which is crucial for these methods, is also correctly implemented.
signal_name_from_numberconverts a signal number to a signal name usingvalidate_signal.signal_number_from_exit_statusconverts an exit status to a signal number, supporting multiple offsets and usingvalidate_signal.The usage of these methods in various files confirms their integration into the system.
yash-env/src/trap.rsyash-env/src/trap/cond.rsyash-env/src/job/fmt.rsyash-builtin/src/trap/cond.rsyash-builtin/src/kill/print.rsyash-builtin/src/kill/signal.rsThese methods are used to handle signals and exit statuses correctly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct implementation of signal number and name conversion methods. # Test: Search for usages of these methods to ensure they are used correctly. rg --type rust $'signal_name_from_number|signal_number_from_exit_status'Length of output: 1626
Script:
#!/bin/bash # Description: Verify the correct implementation of signal number and name conversion methods. # Test: Extract the implementation of `signal_name_from_number` and `signal_number_from_exit_status` methods. ast-grep --lang rust --pattern $'fn signal_name_from_number(&self, number: signal::Number) -> signal::Name { $_$ }' yash-env/src/system.rs ast-grep --lang rust --pattern $'fn signal_number_from_exit_status(&self, status: ExitStatus) -> Option<signal::Number> { $_$ }' yash-env/src/system.rs # Test: Extract the implementation of `validate_signal` method used in both methods. ast-grep --lang rust --pattern $'fn validate_signal(&self, raw_number: i32) -> Result<(signal::Name, signal::Number), Error> { $_$ }' yash-env/src/system.rsLength of output: 456
Script:
#!/bin/bash # Description: Verify the correct usage of `signal_name_from_number` and `signal_number_from_exit_status` methods. # Test: Extract the usage of `signal_name_from_number` in identified files. rg --type rust 'signal_name_from_number' yash-env/src/trap.rs yash-env/src/trap/cond.rs yash-env/src/job/fmt.rs yash-builtin/src/trap/cond.rs yash-builtin/src/kill/print.rs yash-builtin/src/kill/signal.rs # Test: Extract the usage of `signal_number_from_exit_status` in identified files. rg --type rust 'signal_number_from_exit_status' yash-env/src/trap.rs yash-env/src/trap/cond.rs yash-env/src/job/fmt.rs yash-builtin/src/trap/cond.rs yash-builtin/src/kill/print.rs yash-builtin/src/kill/signal.rsLength of output: 1488
Script:
#!/bin/bash # Description: Re-verify the correct implementation of signal number and name conversion methods. # Test: Extract the implementation of `signal_name_from_number` method. rg --type rust 'fn signal_name_from_number' yash-env/src/system.rs -A 10 # Test: Extract the implementation of `signal_number_from_exit_status` method. rg --type rust 'fn signal_number_from_exit_status' yash-env/src/system.rs -A 10 # Test: Extract the implementation of `validate_signal` method used in both methods. rg --type rust 'fn validate_signal' yash-env/src/system.rs -A 10Length of output: 2433
Line range hint
269-298: Review the implementation ofsigactionandcaught_signals.Verification successful
To verify the correct implementation and usage of the
sigactionandcaught_signalsfunctions, we need to review their definitions and usages in the relevant files. The search results indicate that these functions are defined and used in multiple files, includingsystem.rs,virtual.rs, andreal.rs.Let's start by examining the implementation of these functions in
system.rs,virtual.rs, andreal.rs.
The implementations of the
sigactionandcaught_signalsfunctions have been extracted from the relevant files. Here are the details:
In
system.rs, thecaught_signalsfunction is defined as:fn caught_signals(&mut self) -> Vec<signal::Number> { self.0.borrow_mut().caught_signals() }In
virtual.rs, thecaught_signalsfunction is defined as:fn caught_signals(&mut self) -> Vec<signal::Number> { std::mem::take(&mut self.current_process_mut().caught_signals) }In
real.rs, thecaught_signalsfunction is defined as:fn caught_signals(&mut self) -> Vec<signal::Number> { let mut signals = Vec::new(); for slot in &CAUGHT_SIGNALS { // Need a fence to ensure we examine the slots in order. compiler_fence(Ordering::Acquire); let signal = slot.swap(0, Ordering::Relaxed); if signal == 0 { // The `catch_signal` function always fills the first unused // slot, so there is no more slot filled with a signal. break; } if let Some((_name, number)) = self.validate_signal(signal as signal::RawNumber) { signals.push(number) } else { // ignore unknown signal } } signals }The
sigactionfunction implementations were not captured in the output, indicating they might be defined differently or in a more complex manner. To ensure we have a complete understanding, we should re-run the script to capture thesigactionimplementations specifically.
The previous attempt to extract the
sigactionfunction implementations did not produce any output. This suggests that the pattern used might not have matched the actual function definitions, or the functions might be defined in a more complex manner.To ensure we capture the
sigactionimplementations correctly, let's refine our approach by searching for the function definitions usingrgwith a more flexible pattern.
The
sigactionandcaught_signalsfunctions have been successfully extracted from the relevant files. Here are the details:
In
system.rs, thesigactionfunction is defined as:fn sigaction( &mut self, signal: signal::Number, action: SignalHandling, ) -> Result<SignalHandling> { self.0.borrow_mut().sigaction(signal, action) }In
virtual.rs, thesigactionfunction is defined as:fn sigaction( &mut self, signal: signal::Number, action: SignalHandling, ) -> Result<SignalHandling> { let mut process = self.current_process_mut(); Ok(process.set_signal_handling(signal, action)) }In
real.rs, thesigactionfunction is defined as:fn sigaction( &mut self, signal: signal::Number, handling: SignalHandling, ) -> Result<SignalHandling> { unsafe { let new_action = handling.to_sigaction(); let mut old_action = MaybeUninit::<nix::libc::sigaction>::uninit(); let old_mask_ptr = std::ptr::addr_of_mut!((*old_action.as_mut_ptr()).sa_mask); // POSIX requires *all* sigset_t objects to be initialized before use. nix::libc::sigemptyset(old_mask_ptr).errno_if_m1()?; nix::libc::sigaction( signal.as_raw(), new_action.as_ptr(), old_action.as_mut_ptr(), ) .errno_if_m1()?; let old_handling = SignalHandling::from_sigaction(&old_action);The implementations of both
sigactionandcaught_signalsfunctions appear to be consistent and correctly handle signal actions and caught signals. Thesigactionfunction sets the signal handler, andcaught_signalsretrieves the list of caught signals.Based on the extracted implementations, the functions seem to be implemented correctly and used consistently across the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct implementation and usage of signal action setting and signal catching. # Test: Search for usages of these functions to ensure they are used correctly and that signal handling is consistent. rg --type rust $'sigaction|caught_signals'Length of output: 7653
Script:
#!/bin/bash # Extract the implementation of sigaction and caught_signals functions from system.rs, virtual.rs, and real.rs # Extract from system.rs ast-grep --lang rust --pattern $'fn sigaction($_, $_) { $$$ }' yash-env/src/system.rs ast-grep --lang rust --pattern $'fn caught_signals(&mut self) -> Vec<signal::Number> { $$$ }' yash-env/src/system.rs # Extract from virtual.rs ast-grep --lang rust --pattern $'fn sigaction($_, $_) { $$$ }' yash-env/src/system/virtual.rs ast-grep --lang rust --pattern $'fn caught_signals(&mut self) -> Vec<signal::Number> { $$$ }' yash-env/src/system/virtual.rs # Extract from real.rs ast-grep --lang rust --pattern $'fn sigaction($_, $_) { $$$ }' yash-env/src/system/real.rs ast-grep --lang rust --pattern $'fn caught_signals(&mut self) -> Vec<signal::Number> { $$$ }' yash-env/src/system/real.rsLength of output: 2522
Script:
#!/bin/bash # Extract the implementation of sigaction functions from system.rs, virtual.rs, and real.rs # Extract from system.rs ast-grep --lang rust --pattern $'fn sigaction($_, $_) { $$$ }' yash-env/src/system.rs # Extract from virtual.rs ast-grep --lang rust --pattern $'fn sigaction($_, $_) { $$$ }' yash-env/src/system/virtual.rs # Extract from real.rs ast-grep --lang rust --pattern $'fn sigaction($_, $_) { $$$ }' yash-env/src/system/real.rsLength of output: 279
Script:
#!/bin/bash # Extract the implementation of sigaction functions from system.rs, virtual.rs, and real.rs using rg # Extract from system.rs rg --type rust 'fn sigaction' yash-env/src/system.rs -A 20 # Extract from virtual.rs rg --type rust 'fn sigaction' yash-env/src/system/virtual.rs -A 20 # Extract from real.rs rg --type rust 'fn sigaction' yash-env/src/system/real.rs -A 20Length of output: 3390
311-311: Ensure thekillfunction correctly handles the optional signal parameter.Verification successful
The
killfunction implementations inyash-env/src/system.rs,yash-env/src/system/real.rs, andyash-env/src/system/virtual.rshave been successfully extracted. Here are the key points:
- In
yash-env/src/system.rs, thekillfunction delegates the call to anotherkillfunction, passing thetargetandsignalparameters.- In
yash-env/src/system/real.rs, thekillfunction uses thenix::libc::killfunction, correctly handling the optionalsignalparameter by mapping it to a raw value.- In
yash-env/src/system/virtual.rs, thekillfunction handles thesignalparameter and sends it to processes based on thetarget.These implementations indicate that the
killfunction correctly handles the optionalsignalparameter.
yash-env/src/system.rsyash-env/src/system/real.rsyash-env/src/system/virtual.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct implementation of the `kill` function with optional signal parameter. # Test: Search for usages of the `kill` function to ensure it is used correctly. rg --type rust $'kill'Length of output: 5915
Script:
#!/bin/bash # Extract the implementation of the `kill` function from `yash-env/src/system.rs` ast-grep --lang rust --pattern $'fn kill($_, $_) { $$$ }' yash-env/src/system.rsLength of output: 83
Script:
#!/bin/bash # Search for the definition of the `kill` function across the entire codebase ast-grep --lang rust --pattern $'fn kill($_, $_) { $$$ }'Length of output: 60
Script:
#!/bin/bash # Search for the definition of the `kill` function across the entire codebase using `rg` rg 'fn kill'Length of output: 735
Script:
#!/bin/bash # Extract the implementation of the `kill` function from relevant files ast-grep --lang rust --pattern $'fn kill($_, $_) { $$$ }' yash-env/src/system.rs yash-env/src/system/real.rs yash-env/src/system/virtual.rsLength of output: 142
Script:
#!/bin/bash # Search for the definition of the `kill` function in specific files using `rg` rg 'fn kill' yash-env/src/system.rs yash-env/src/system/real.rs yash-env/src/system/virtual.rsLength of output: 601
Script:
#!/bin/bash # Extract the implementation of the `kill` function from specific files rg -A 10 'fn kill' yash-env/src/system.rs yash-env/src/system/real.rs yash-env/src/system/virtual.rsLength of output: 6575
yash-env/src/system/virtual.rs (12)
635-639: Validate the new signal handling functions for correctness.The implementation of
validate_signalcorrectly checks for a non-zero signal number and attempts to convert it to asignal::Nameandsignal::Number. This aligns with the PR's objective to handle signals more robustly.
641-644: Check the implementation ofsignal_number_from_name.The method
signal_number_from_namecorrectly retrieves the raw virtual number from a signal name. This is a straightforward and efficient implementation.
673-677: Review changes tosigactionmethod.The updated
sigactionmethod now correctly handles the new signal handling structure, which is part of the PR's objectives to enhance signal handling.
697-697: Examine the modifications to thekillmethod.The
killmethod has been updated to use the newsignal::Numbertype, which is consistent with the rest of the changes in the PR. The method's logic remains correct and aligns with the expected behavior.
1078-1078: Check the helper functionsend_signal_to_processes.The function
send_signal_to_processeshas been correctly updated to handle the new signal types. It iterates over processes and sends signals appropriately, which is crucial for the virtual system's operation.
1105-1105: Review theraise_sigchldfunction.The
raise_sigchldfunction is correctly raising theSIGCHLDsignal without changing the process state, which is used to notify the parent process of changes in the child process's state.
1840-1846: Ensure correct handling of signals in thekillmethod.The
killmethod's handling ofSIGINTand its impact on the process state is correctly implemented. The method's behavior aligns with the expected outcomes described in the PR.
1856-1856: Verify error handling in thekillmethod.The method correctly handles the case where a non-existent process ID is targeted, returning the appropriate error code
ESRCH.
1881-1889: Check the behavior ofkillwhen sendingSIGTERMto all processes.The method correctly handles the scenario where
SIGTERMis sent to all processes, ensuring that each process's state is updated appropriately.
1915-1937: Review the handling ofSIGQUITin thekillmethod.The method correctly handles the scenario where
SIGQUITis sent to all processes in the same group, ensuring that each process's state is updated appropriately, including the correct handling of core dumps.
Line range hint
1963-1983: Examine the handling ofSIGHUPin thekillmethod.The method correctly handles the scenario where
SIGHUPis sent to a specific process group, ensuring that each affected process's state is updated appropriately.
2001-2001: Check the handling ofSIGCONTin thekillmethod.The method correctly handles the scenario where
SIGCONTis sent to a process group, ensuring that the process state remains running if it was already running.
| @@ -250,7 +249,7 @@ impl Env { | |||
| /// [`wait_for_signals`](Self::wait_for_signals) but does not wait for | |||
| /// signals to be caught. Instead, it only checks if any signals have been | |||
| /// caught but not yet consumed in the [`SharedSystem`]. | |||
| pub fn poll_signals(&mut self) -> Option<Rc<[Signal]>> { | |||
| pub fn poll_signals(&mut self) -> Option<Rc<[signal::Number]>> { | |||
There was a problem hiding this comment.
Review efficiency of poll_signals.
Consider optimizing the poll_signals method to reduce overhead by minimizing context switches and potentially using more efficient polling mechanisms.
| - `Env::wait_for_signal` | ||
| - `Env::wait_for_signals` | ||
| - `job::ProcessState::stopped` | ||
| - `system::System::caught_signals` | ||
| - `system::System::kill` | ||
| - `system::System::sigaction` | ||
| - `system::virtual::Process::signal_handling` | ||
| - `system::virtual::Process::set_signal_handling` | ||
| - `trap::TrapSet::catch_signal` | ||
| - `trap::TrapSet::take_caught_signal` | ||
| - `trap::TrapSet::take_signal_if_caught` | ||
| - `system::virtual::SignalEffect::of` now takes a `signal::Number` parameter |
There was a problem hiding this comment.
Adjust the indentation of the unordered list to match the expected level.
- - `Env::wait_for_signal`
- - `Env::wait_for_signals`
- - `job::ProcessState::stopped`
- - `system::System::caught_signals`
- - `system::System::kill`
- - `system::System::sigaction`
- - `system::virtual::Process::signal_handling`
- - `system::virtual::Process::set_signal_handling`
- - `trap::TrapSet::catch_signal`
- - `trap::TrapSet::take_caught_signal`
- - `trap::TrapSet::take_signal_if_caught`
+ - `Env::wait_for_signal`
+ - `Env::wait_for_signals`
+ - `job::ProcessState::stopped`
+ - `system::System::caught_signals`
+ - `system::System::kill`
+ - `system::System::sigaction`
+ - `system::virtual::Process::signal_handling`
+ - `system::virtual::Process::set_signal_handling`
+ - `trap::TrapSet::catch_signal`
+ - `trap::TrapSet::take_caught_signal`
+ - `trap::TrapSet::take_signal_if_caught`Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - `Env::wait_for_signal` | |
| - `Env::wait_for_signals` | |
| - `job::ProcessState::stopped` | |
| - `system::System::caught_signals` | |
| - `system::System::kill` | |
| - `system::System::sigaction` | |
| - `system::virtual::Process::signal_handling` | |
| - `system::virtual::Process::set_signal_handling` | |
| - `trap::TrapSet::catch_signal` | |
| - `trap::TrapSet::take_caught_signal` | |
| - `trap::TrapSet::take_signal_if_caught` | |
| - `system::virtual::SignalEffect::of` now takes a `signal::Number` parameter | |
| - `Env::wait_for_signal` | |
| - `Env::wait_for_signals` | |
| - `job::ProcessState::stopped` | |
| - `system::System::caught_signals` | |
| - `system::System::kill` | |
| - `system::System::sigaction` | |
| - `system::virtual::Process::signal_handling` | |
| - `system::virtual::Process::set_signal_handling` | |
| - `trap::TrapSet::catch_signal` | |
| - `trap::TrapSet::take_caught_signal` | |
| - `trap::TrapSet::take_signal_if_caught` | |
| - `system::virtual::SignalEffect::of` now takes a `signal::Number` parameter |
Tools
Markdownlint
40-40: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
41-41: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
42-42: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
43-43: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
44-44: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
45-45: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
46-46: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
47-47: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
48-48: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
49-49: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
50-50: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
yash-env/src/system.rs
Outdated
| #[inline] | ||
| fn signal_name_from_number(&self, number: signal::Number) -> signal::Name { | ||
| SystemEx::signal_name_from_number(self, number) | ||
| } | ||
|
|
||
| #[inline] | ||
| fn signal_number_from_name(&self, name: signal::Name) -> signal::Number { | ||
| System::signal_number_from_name(self, name) | ||
| .unwrap_or_else(|| panic!("unsupported signal name: {name:?}")) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The implementations of the methods signal_name_from_number and signal_number_from_name have been extracted. Here's the analysis:
-
signal_name_from_number:- In
yash-env/src/system.rs(line 683), it usesself.validate_signal(number.as_raw()).unwrap().0, which could panic if the signal is not valid. - In
yash-env/src/system.rs(line 1114), it delegates toSystemEx::signal_name_from_number.
- In
-
signal_number_from_name:- In
yash-env/src/system.rs(line 1120), it usesunwrap_or_else(|| panic!("unsupported signal name: {name:?}")), which explicitly panics on unsupported signal names. - Other implementations return
Option<signal::Number>, which is safer as it allows handling of unsupported signals without panicking.
- In
The methods in yash-env/src/system.rs do not handle unsupported signals gracefully and can cause panics. This confirms the review comment's concern.
yash-env/src/system.rs:- Line 683:
self.validate_signal(number.as_raw()).unwrap().0 - Line 1120:
System::signal_number_from_name(self, name).unwrap_or_else(|| panic!("unsupported signal name: {name:?}"))
- Line 683:
Analysis chain
Ensure that the signal_name_from_number and signal_number_from_name methods handle unsupported signals correctly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the correct handling of unsupported signals in conversion methods.
# Test: Search for usages of these methods to ensure they handle errors correctly.
rg --type rust $'signal_name_from_number|signal_number_from_name'
Length of output: 4444
Script:
#!/bin/bash
# Extract the implementation of signal_name_from_number and signal_number_from_name methods
ast-grep --lang rust --pattern 'fn signal_name_from_number($_, $_) { $$$ }'
ast-grep --lang rust --pattern 'fn signal_number_from_name($_, $_) { $$$ }'
# Extract the usages of these methods to check for error handling
rg 'signal_name_from_number'
rg 'signal_number_from_name'
Length of output: 5615
Script:
#!/bin/bash
# Extract the full implementation of signal_name_from_number and signal_number_from_name methods
ast-grep --lang rust --pattern 'fn signal_name_from_number($_, $_) -> $_ { $$$ }'
ast-grep --lang rust --pattern 'fn signal_number_from_name($_, $_) -> $_ { $$$ }'
Length of output: 2136
This commit modifies the `SignalSystem::signal_number_from_name` method to return `Option<signal::Number>` instead of `signal::Number`. This change is made to match the behavior of the method with the `System::signal_number_from_name` method. Previously, these methods had slightly different behaviors, which was confusing.
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- yash-builtin/src/trap/syntax.rs (14 hunks)
- yash-env/src/system.rs (23 hunks)
- yash-env/src/trap.rs (65 hunks)
Files not summarized due to errors (1)
- yash-env/src/trap.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (1)
- yash-builtin/src/trap/syntax.rs
Additional context used
Learnings (1)
yash-env/src/system.rs (2)
User: magicant PR: magicant/yash-rs#367 File: yash-env/src/system.rs:57-57 Timestamp: 2024-06-08T02:51:51.839Z Learning: The `yash-env/src/subshell.rs` module depends on `SigSet`, which requires `nix::sys::signal::Signal`. Plans are in place to remove `SigSet` in a future PR, and some existing methods like `tcsetpgrp_with_block` still depend on `nix::sys::signal::Signal`.User: magicant PR: magicant/yash-rs#367 File: yash-env/src/subshell.rs:40-40 Timestamp: 2024-06-08T02:49:23.406Z Learning: The `yash-env/src/subshell.rs` module depends on `SigSet`, which requires `nix::sys::signal::Signal`. Plans are in place to remove `SigSet` in a future PR.
Additional comments not posted (9)
yash-env/src/trap.rs (1)
54-54: Ensure that the system supports all required signals.Also applies to: 74-75
Verification successful
The implementations of the
signal_number_from_namefunction across different files indicate that the function delegates the signal name to number conversion to other methods (to_raw_virtualandto_raw_real). This suggests that the actual signal handling logic is encapsulated within these methods.To ensure that all required signals are supported, we need to verify the implementations of the
to_raw_virtualandto_raw_realmethods. Let's extract the details of these methods to confirm their completeness.
The absence of output from the
ast-grepcommands suggests that theto_raw_virtualandto_raw_realmethods might not be directly implemented in the files we searched. They could be defined elsewhere or implemented differently.To proceed, we should search for the definitions of these methods across the entire codebase to ensure we capture their implementations accurately.
The
to_raw_virtualandto_raw_realmethods are defined inyash-env/src/system/virtual/signal.rsandyash-env/src/system/real/signal.rs, respectively. To ensure that all required signals are supported, we need to extract the implementation details of these methods.
The absence of output from the
ast-grepcommands suggests that the methodsto_raw_virtualandto_raw_realmight be implemented differently or might not match the exact pattern we used. To ensure we capture their implementations accurately, we should search for the method bodies usingrgto locate the exact lines and then extract the surrounding context.
The extracted context for the
to_raw_virtualandto_raw_realmethods shows that they handle various signal names and convert them to their corresponding signal numbers. Theto_raw_virtualmethod supports a range of real-time signals, while theto_raw_realmethod maps specific signal names to their raw numbers using thenix::libcconstants.Based on this information, it appears that the system does support the required signals through these methods. Therefore, the concern about ensuring that the system supports all required signals is addressed.
to_raw_virtualmethod inyash-env/src/system/virtual/signal.rshandles a range of real-time signals.to_raw_realmethod inyash-env/src/system/real/signal.rsmaps specific signal names to their raw numbers.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all required signals are supported by the system. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type rust $'fn signal_number_from_name'Length of output: 819
Script:
#!/bin/bash # Extract the implementation details of the `signal_number_from_name` function from the identified files. # Extract from yash-env/src/trap.rs ast-grep --lang rust --pattern $'fn signal_number_from_name(&self, name: signal::Name) -> Option<signal::Number> { $$$ }' yash-env/src/trap.rs # Extract from yash-env/src/system.rs ast-grep --lang rust --pattern $'fn signal_number_from_name(&self, name: signal::Name) -> Option<signal::Number> { $$$ }' yash-env/src/system.rs # Extract from yash-env/src/system/virtual.rs ast-grep --lang rust --pattern $'fn signal_number_from_name(&self, name: signal::Name) -> Option<signal::Number> { $$$ }' yash-env/src/system/virtual.rs # Extract from yash-env/src/system/real.rs ast-grep --lang rust --pattern $'fn signal_number_from_name(&self, name: signal::Name) -> Option<signal::Number> { $$$ }' yash-env/src/system/real.rsLength of output: 1740
Script:
#!/bin/bash # Extract the implementation details of the `to_raw_virtual` and `to_raw_real` methods. # Extract `to_raw_virtual` method ast-grep --lang rust --pattern $'fn to_raw_virtual(&self) -> Option<signal::Number> { $$$ }' yash-env/src/system/virtual.rs # Extract `to_raw_real` method ast-grep --lang rust --pattern $'fn to_raw_real(&self) -> Option<signal::Number> { $$$ }' yash-env/src/system/real.rsLength of output: 247
Script:
#!/bin/bash # Search for the definitions of the `to_raw_virtual` and `to_raw_real` methods across the entire codebase. # Search for `to_raw_virtual` method rg --type rust 'fn to_raw_virtual' # Search for `to_raw_real` method rg --type rust 'fn to_raw_real'Length of output: 258
Script:
#!/bin/bash # Extract the implementation details of the `to_raw_virtual` and `to_raw_real` methods. # Extract `to_raw_virtual` method ast-grep --lang rust --pattern $'fn to_raw_virtual(self) -> Option<Number> { $$$ }' yash-env/src/system/virtual/signal.rs # Extract `to_raw_real` method ast-grep --lang rust --pattern $'fn to_raw_real(self) -> Option<Number> { $$$ }' yash-env/src/system/real/signal.rsLength of output: 243
Script:
#!/bin/bash # Search for the definitions and surrounding context of the `to_raw_virtual` and `to_raw_real` methods. # Search for `to_raw_virtual` method with surrounding context rg --context 10 'fn to_raw_virtual' yash-env/src/system/virtual/signal.rs # Search for `to_raw_real` method with surrounding context rg --context 10 'fn to_raw_real' yash-env/src/system/real/signal.rsLength of output: 1608
yash-env/src/system.rs (8)
57-57: The import ofnix::sys::signal::Signalis still necessary for handlingSigSetin methods liketcsetpgrp_with_blockas per the discussion in previous comments.
219-228: The new methodvalidate_signalis well-implemented. It provides a clear and concise way to validate signal numbers, returning the most common name for a signal number if multiple names exist.
269-273: Thesigactionmethod is correctly updated to use the newsignal::Numbertype. This change aligns with the PR's goal to use the new signal handling types.
311-311: Thekillmethod update to useOption<signal::Number>is appropriate and aligns with the new signal handling strategy.
673-698: The methodssignal_name_from_numberandsignal_number_from_exit_statusare well-implemented, providing robust functionality for signal handling. These methods are essential for the correct interpretation of signals and exit statuses.
643-665: The methodtcsetpgrp_without_blockuses the newsignal_number_from_nameto handleSIGTTOU. This is a critical update for safely changing the foreground process group. Ensure thatSIGTTOUis correctly handled in all relevant scenarios.Verification successful
The search results indicate that
SIGTTOUis referenced and handled in multiple files, includingjob.rs,system.rs,trap.rs,signal.rs,subshell.rs, andstate.rs. The handling ofSIGTTOUappears to be consistent with the expected behavior, particularly in the context of changing the foreground process group and managing signal masks.Key points of interest include:
system.rs: Functions liketcsetpgrp_with_blockandtcsetpgrp_without_blockhandleSIGTTOUby blocking and unblocking the signal appropriately.trap.rsandtrap/state.rs: Various tests and functions ensure thatSIGTTOUis set toIgnoreorDefaultas needed.job.rs: Updates the process state tostopped(SIGTTOU)in relevant scenarios.Given the comprehensive handling and testing of
SIGTTOUacross these files, the implementation appears robust.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct handling of `SIGTTOU` in `tcsetpgrp_without_block`. # Test: Check all scenarios where `SIGTTOU` should be handled. Expect: Correct behavior in all cases. echo "Check all scenarios where `SIGTTOU` should be handled."Length of output: 186
Script:
#!/bin/bash # Description: Search for all instances where `SIGTTOU` is handled in the codebase. # Find all occurrences of `SIGTTOU` in the codebase rg 'SIGTTOU'Length of output: 5592
Script:
#!/bin/bash # Description: Extract and review relevant code snippets for `SIGTTOU` handling. # Extract code snippets from files where `SIGTTOU` is referenced rg -A 5 -B 5 'SIGTTOU' yash-env/src/job.rs yash-env/src/system.rs yash-env/src/trap.rs yash-env/src/signal.rs yash-env/src/subshell.rs yash-env/src/trap/state.rsLength of output: 30447
229-234: The methodsignal_number_from_namecorrectly retrieves the signal number based on the signal name. Ensure that all signal names used in the system are supported to avoid returningNone.
298-298: The methodcaught_signalsis crucial for handling caught signals. It's important to ensure that signals are not lost if they are caught when the list is full.
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)
- yash-builtin/src/kill/syntax.rs (24 hunks)
Additional context used
Learnings (1)
yash-builtin/src/kill/syntax.rs (2)
User: magicant PR: magicant/yash-rs#367 File: yash-env/src/system.rs:57-57 Timestamp: 2024-06-08T02:51:51.839Z Learning: The `yash-env/src/subshell.rs` module depends on `SigSet`, which requires `nix::sys::signal::Signal`. Plans are in place to remove `SigSet` in a future PR, and some existing methods like `tcsetpgrp_with_block` still depend on `nix::sys::signal::Signal`.User: magicant PR: magicant/yash-rs#367 File: yash-env/src/subshell.rs:40-40 Timestamp: 2024-06-08T02:49:23.406Z Learning: The `yash-env/src/subshell.rs` module depends on `SigSet`, which requires `nix::sys::signal::Signal`. Plans are in place to remove `SigSet` in a future PR.
Additional comments not posted (7)
yash-builtin/src/kill/syntax.rs (7)
25-25: Import ofSignaltype aligns with the PR's objective to use the new signal handling system.
28-28: Import ofyash_env::signalis necessary for the new signal handling logic.
359-395: Updated tests to reflect changes in signal parsing logic.These updates are necessary to ensure that the new signal handling logic is correctly implemented and behaves as expected.
597-604: The test for option-lwith operands has been updated to use the new signal types.This change ensures that the tests are aligned with the new signal handling logic.
Line range hint
792-822: Added tests for invalid signal arguments, ensuring robust error handling.These tests are crucial for verifying that the error handling logic correctly identifies invalid signals, enhancing the robustness of the command parsing.
193-195: Updatedset_signalandparse_signalsfunctions to work with the newSignaltype.These changes are crucial for integrating the new signal handling system. Verify that these functions are used consistently across the codebase to maintain uniformity in signal handling.
Also applies to: 222-225
Verification successful
The search results indicate that the
set_signalandparse_signalsfunctions are defined and used within theyash-builtin/src/kill/syntax.rsfile. There are no other instances of these functions being used outside this file within theyash-builtindirectory. This suggests that the changes to these functions are localized and do not impact other parts of the codebase.
set_signalis defined and used three times withinyash-builtin/src/kill/syntax.rs.parse_signalsis defined and used once withinyash-builtin/src/kill/syntax.rs.Given this information, it appears that the functions are used consistently within the file where they are defined.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of `set_signal` and `parse_signals` across the codebase. # Test: Search for the function usage. Expect: Consistent usage of the new signal handling logic. rg --type rust $'set_signal|parse_signals'Length of output: 4096
Script:
#!/bin/bash # Description: Verify consistent usage of `set_signal` and `parse_signals` functions within the yash-builtin directory. # Test: Search for the specific usage of `set_signal` and `parse_signals` functions. rg --type rust 'set_signal|parse_signals' yash-builtin/Length of output: 570
Line range hint
256-349: Significant refactoring in theparsefunction to accommodate the new signal handling logic.This refactoring is extensive and touches many aspects of command parsing. It's crucial to ensure that all edge cases are handled correctly, especially with the new signal types.
Verification successful
The search results indicate that there are indeed tests for the
parse_signalfunction inyash-builtin/src/kill/syntax.rs. These tests cover various scenarios, including valid signal names, signal numbers, and invalid inputs.
parse_signal("INT", false)parse_signal("RtMin+5", false)parse_signal("SigRtMin+5", false)parse_signal("0", false)parse_signal("1", false)parse_signal("TERM1", false)parse_signal("", false)These tests suggest that edge cases are being handled, such as:
- Valid signal names with and without the "SIG" prefix.
- Valid signal numbers.
- Invalid signal names and numbers.
- Empty strings.
Given this information, it appears that the edge cases for signal parsing are adequately covered by the existing tests.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all edge cases in signal parsing are handled correctly. # Test: Search for tests covering edge cases in signal parsing. rg --type rust $'parse_signal|parse'Length of output: 228275
Script:
#!/bin/bash # Description: Verify that all edge cases in signal parsing are handled correctly by searching for specific test cases. # Test: Search for test cases related to signal parsing in the test files. rg --type rust --glob '*test*' 'parse_signal\(|parse\('Length of output: 475
Script:
#!/bin/bash # Description: Verify that all edge cases in signal parsing are handled correctly by searching for specific test cases. # Test: Search for test cases specifically related to `parse_signal` and signal parsing in test files. rg --type rust --glob '*test*' 'parse_signal\('Length of output: 49
Script:
#!/bin/bash # Description: Verify that all edge cases in signal parsing are handled correctly by searching for specific test cases. # Test: Search for test cases related to signal parsing across all files. rg --type rust 'parse_signal\('Length of output: 2043
This is part of #353, and rework of #365.
This PR removes the use of
nix::signal::Signalin theyash_env::SystemAPI. The definition ofSignalis dependent on the underlying system, so it should not be used outsideRealSystem. Instead, new neutral items are added in theyash_env::signalmodule.The new
Numbertype replaces the most use ofSignal. TheRealSystemandVirtualSystemnow have differentNumberspaces so that theVirtualSystemno longer has to depend on theRealSystem's signal number. The other uses ofSignals are replaced byNames, which is more abstract thanNumbers.This PR also adds (partial) support for real-time signals.
To fully support real-time signals, we also need to eliminate the use of
SigSet, which is not the scope of this pull request.NumberandName)From<Number>forExitStatusSystem::signal_number_from_nameandSystem::validate_signal_numberSystemEx:signal_name_from_numberSystemEx:signal_number_from_exit_statuswhich depends onSystemand will replaceyash_env::job::fmtNumberin virtualProcessProcessResultcontainNumberand update the implementation ofFrom<ProcessResult>forExitStatus, etc.yash::job::fmt, which depends on the oldProcessResultdefinition.yash_env::trap::ConditioncontainNumberand update dependent implementations accordinglySystem::sigactionSystem::caught_signalsSystem::killSelectSystem::set_signal_handlingSharedSystem::wait_for_signalsSharedSystem::wait_for_signalyash::job::fmtitemsProcessState::from_wait_statusby inlining the functionimpl TryFrom<Signal> for signal::Numberimpl From<Signal> for ExitStatusyash_builtin::trap::Command::executeinto smaller functionsSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation