Replace SigSet with Signal array in System #368
Conversation
This commit replaces the SigSet type with an array of Signal in the System trait. This change is necessary because the SigSet type is specific to the nix crate, and the System trait should not depend on specific types from external crates. This commit also updates the implementations of the System trait to reflect this change. SelectSystem::wait_mask is now an optional Vec of Signal, and virtual Process now uses a BTreeSet of Signal instead of SigSet.
WalkthroughThe changes in this update focus on improving signal handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Subshell
participant System
participant Signal
User->>Subshell: block_sigint_sigquit()
Subshell->>System: sigmask(Some((SIG_BLOCK, &[sigint, sigquit])), Some(&mut old_mask))
System->>Signal: Retrieve signal numbers
Signal-->>System: Return signal numbers
System-->>Subshell: Block signals and update old_mask
Subshell-->>User: Return success/failure
Possibly related issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- yash-env/CHANGELOG.md (1 hunks)
- yash-env/src/lib.rs (3 hunks)
- yash-env/src/subshell.rs (5 hunks)
- yash-env/src/system.rs (13 hunks)
- yash-env/src/system/real.rs (3 hunks)
- yash-env/src/system/real/signal.rs (1 hunks)
- yash-env/src/system/virtual.rs (6 hunks)
- yash-env/src/system/virtual/process.rs (12 hunks)
- yash-env/src/system/virtual/signal.rs (2 hunks)
Additional context used
Learnings (7)
Common learnings
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`.yash-env/CHANGELOG.md (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.yash-env/src/system/real.rs (1)
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`.yash-env/src/lib.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.yash-env/src/subshell.rs (2)
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.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`.yash-env/src/system/virtual/process.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.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.
LanguageTool
yash-env/CHANGELOG.md
[uncategorized] ~60-~60: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...-system::virtual::SignalEffect::ofnow takes asignal::Number` parameter in...
Markdownlint
yash-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
51-51: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
52-52: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
53-53: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
54-54: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
55-55: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
Additional comments not posted (24)
yash-env/CHANGELOG.md (1)
46-57: Ensure the new parameters insystem::System::sigmaskare correctly documented and used throughout the codebase.Tools
Markdownlint
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
51-51: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
52-52: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
53-53: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
54-54: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
55-55: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentationyash-env/src/system/real/signal.rs (1)
402-423: The implementation ofall_signalsandsigset_to_veclooks correct. Ensure that the use ofunsafeinsigset_to_vecis justified and safe.yash-env/src/system/virtual/signal.rs (1)
Line range hint
1-129: The constants for signal numbers in the virtual system are correctly defined usingunsafe. Ensure that these values are consistent with the system's requirements and that the use ofunsafeis justified.yash-env/src/system/real.rs (2)
360-403: Refactorsigmaskmethod to usesignal::Numberinstead ofSigSet.The refactoring of the
sigmaskmethod aligns with the PR's objective to remove dependencies on thenixcrate'sSigSet. The use ofMaybeUninitforsigset_tand the handling oferrnoare correctly implemented. Ensure that all callers of this method are updated to use the new signature.
470-496: Updateselectmethod to usesignal::Numberarray.The update to the
selectmethod to accept an array ofsignal::Numberinstead ofSigSetis consistent with the overall changes in the PR. The use ofMaybeUninitand proper error handling witherrno_if_m1are correctly applied. Verify that all usages of this method across the codebase have been updated to pass the correct type.Verification successful
The output from the shell scripts shows multiple instances of the
selectfunction across various files. The relevant function signatures and calls need to be verified to ensure they match the new signature that accepts an array ofsignal::Numberinstead ofSigSet.Observations:
Function Signatures:
yash-env/src/system.rs:fn select( &mut self, readers: &mut FdSet, writers: &mut FdSet, timeout: Option<&TimeSpec>, signal_mask: Option<&[signal::Number]>, ) -> Result<c_int>;yash-env/src/system/virtual.rs:fn select( &mut self, readers: &mut FdSet, writers: &mut FdSet, timeout: Option<&TimeSpec>, signal_mask: Option<&[signal::Number]>, ) -> Result<c_int>;Function Calls:
- Calls in
yash-env/src/system/virtual.rsandyash-env/src/system.rsmatch the new signature:let result = system.select(&mut readers, &mut writers, None, None); let result = system.select(&mut FdSet::new(), &mut fds, None, Some(&[]));Conclusion:
The function signatures and calls to
selectin the relevant files have been updated to match the new signature that accepts an array ofsignal::Number. This confirms that the changes are consistent across the codebase.
yash-env/src/system.rsyash-env/src/system/virtual.rsFinal Response:
The verification confirms that all usages of the
selectmethod across the codebase have been updated to pass the correct type.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `select` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type rust $'select'Length of output: 10202
Script:
#!/bin/bash # Description: Extract function signatures and calls to verify if they match the new signature. # Extract function signatures for `select` in relevant files echo "Function signatures:" rg --type rust --pcre2 --context 5 'fn select' yash-env/src/lib.rs yash-env/src/system.rs yash-env/src/system/virtual.rs # Extract function calls to `select` in relevant files echo "Function calls:" rg --type rust --pcre2 --context 5 '\.select\(' yash-env/src/lib.rs yash-env/src/system.rs yash-env/src/system/virtual.rsLength of output: 29504
yash-env/src/lib.rs (2)
521-521: The testwait_for_signal_remembers_signal_in_trap_setcorrectly checks for the presence ofSIGCHLDinblocked_signals. This aligns with the PR's objective to handle signals using new data structures.
560-560: The testpoll_signals_someeffectively verifies thatSIGCHLDis correctly recognized as a blocked signal, which is consistent with the changes made in the signal handling logic.yash-env/src/subshell.rs (5)
32-32: Added import forcrate::signal.This change aligns with the removal of
SigSetand the transition to usingsignal::Numberdirectly.
265-265: Updatedold_maskto useVec<signal::Number>instead ofSigSet.This change is consistent with the overall PR objective to decouple from the
nixcrate and use more generic types.
284-289: BlockSIGINTandSIGQUITusing the new signal handling approach.This update is necessary to align with the new signal handling mechanism using
Vec<signal::Number>.
303-303: Restore the original signal mask upon droppingMaskGuard.Proper cleanup of signal masks is crucial for preventing unintended signal blocking states. This change ensures that the original state is restored, which is good practice.
659-660: Ensure thatSIGINTandSIGQUITare not blocked in the parent process.This check is important to verify that signal handling changes in the subshell do not inadvertently affect the parent process. Good inclusion for robustness.
yash-env/src/system/virtual/process.rs (8)
84-84: Update toBTreeSet<signal::Number>for signal management aligns with the PR objectives.This change enhances flexibility and decouples the system from specific external crate types, aligning with the PR's goal to remove dependencies on the
nixcrate.Also applies to: 138-138, 154-154, 334-334
87-87: Ensure that the new data structure for pending signals is correctly integrated.The use of
BTreeSetfor pending signals should improve the efficiency of operations like checking for the presence of a signal.Also applies to: 139-139, 155-155, 342-342
645-690: Unit tests for signal blocking and unblocking appear comprehensive.These tests are crucial for ensuring that the new signal handling logic works as expected. It's good to see a thorough testing approach.
Line range hint
845-857: Check the handling of ignored and blocked signals in the test scenario.This test is important for ensuring that signals that are both ignored and blocked are handled correctly in the system.
Verification successful
The test function
process_raise_signal_ignored_and_blocked_sigcontcorrectly simulates the scenario of ignored and blocked signals. It sets up a process, configures the signal handling to ignoreSIGCONT, blocks theSIGCONTsignal, raises the signal, and then verifies the expected outcomes.
- The process is set to a stopped state with
SIGTTOU.- The
SIGCONTsignal is set to be ignored.- The
SIGCONTsignal is blocked.- The
SIGCONTsignal is raised.- The test asserts that the signal was not delivered or caught, but the process state changed.
- It verifies that the process state is
Running, no signals were caught, andSIGCONTis in the pending signals.This confirms that the test handles ignored and blocked signals correctly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the test for ignored and blocked signals correctly simulates the scenario. # Test: Search for the test usage. Expect: Correct simulation of ignored and blocked signals. rg --type rust $'process_raise_signal_ignored_and_blocked_sigcont'Length of output: 164
Script:
#!/bin/bash # Description: Extract the implementation of the `process_raise_signal_ignored_and_blocked_sigcont` function. # Extract the function definition and body. ast-grep --lang rust --pattern $'fn process_raise_signal_ignored_and_blocked_sigcont() { $$$ }' yash-env/src/system/virtual/process.rsLength of output: 1735
355-372: Review theblock_signalsmethod for correct signal handling logic.The method now uses
BTreeSetfor signal management. Ensure that the logic for blocking, unblocking, and delivering signals is correctly implemented and integrates well with other parts of the system.Verification successful
To verify the correct signal handling logic in the
block_signalsmethod, we need to review both the method implementation and its usage in the codebase. Thergoutput shows multiple instances whereblock_signalsis called. Let's inspect these instances to ensure they correctly handle signal sets.
The
block_signalsmethod and its usage have been thoroughly reviewed. The method implementation correctly updates theblocked_signalsset based on thehowparameter and processes thepending_signalsset accordingly. The usage instances in the codebase also demonstrate correct handling of signal sets, including tests that validate the behavior for different signal masking operations.
- The method correctly handles
SIG_SETMASK,SIG_BLOCK, andSIG_UNBLOCKoperations.- The tests confirm that the method updates the
blocked_signalsset and processes signals as expected.No issues were found with the signal handling logic in the
block_signalsmethod.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `block_signals` method correctly updates the blocked and pending signals sets. # Test: Search for the method usage. Expect: Correct handling of signal sets. rg --type rust $'block_signals'Length of output: 1659
Script:
#!/bin/bash # Description: Extract and review the implementation and usage of the `block_signals` method. # Extract the implementation of the `block_signals` method. echo "Implementation of block_signals method:" rg --type rust --context 20 $'pub fn block_signals(&mut self, how: SigmaskHow, signals: &[signal::Number]) -> SignalResult' yash-env/src/system/virtual/process.rs # Extract the usage of the `block_signals` method in the identified files. echo "Usage of block_signals method in yash-env/src/system/virtual.rs:" rg --type rust --context 5 $'block_signals' yash-env/src/system/virtual.rs echo "Usage of block_signals method in yash-env/src/system/virtual/process.rs:" rg --type rust --context 5 $'block_signals' yash-env/src/system/virtual/process.rsLength of output: 5859
Line range hint
881-903: Ensure theprocess_raise_signal_blockedtest accurately tests the blocking logic.This test should accurately reflect the changes made to signal handling, particularly the use of
BTreeSetfor managing blocked signals.Verification successful
The
process_raise_signal_blockedfunction implementation has been successfully retrieved. Let's analyze it to ensure it accurately tests the signal blocking logic.Analysis of
process_raise_signal_blockedFunctionThe function performs the following steps:
- Creates a
Processinstance with a parent and group.- Sets the signal handling for
SIGCHLDtoCatch.- Blocks the
SIGCHLDsignal and verifies that it is not delivered, caught, or changes the process state.- Raises the
SIGCHLDsignal and verifies that it is not delivered, caught, or changes the process state.- Ensures the process state remains
Runningand no signals are caught.- Unblocks all signals and verifies that the previously blocked
SIGCHLDsignal is now delivered and caught, but does not change the process state.- Ensures the process state remains
Runningand theSIGCHLDsignal is caught.Conclusion
The test accurately verifies the blocking and unblocking of signals, ensuring that the
SIGCHLDsignal is handled correctly according to the blocking logic.
- The test blocks the
SIGCHLDsignal and verifies it is not delivered or caught.- It raises the
SIGCHLDsignal while blocked and verifies it is not delivered or caught.- It unblocks the signals and verifies the previously blocked
SIGCHLDsignal is delivered and caught.This confirms that the
process_raise_signal_blockedtest accurately tests the signal blocking logic.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `process_raise_signal_blocked` test correctly tests the signal blocking logic. # Test: Search for the test usage. Expect: Correct testing of signal blocking. rg --type rust $'process_raise_signal_blocked'Length of output: 124
Script:
#!/bin/bash # Extract the implementation of the `process_raise_signal_blocked` function from the `yash-env/src/system/virtual/process.rs` file. ast-grep --lang rust --pattern $'fn process_raise_signal_blocked() { $$$ }' yash-env/src/system/virtual/process.rsLength of output: 2968
696-706: Review the logic in theprocess_sigmask_unblocktest.Ensure that the test accurately reflects the logic for unblocking signals, especially since the underlying data structures have changed.
Verification successful
The implementation of the
process_sigmask_unblockfunction has been successfully extracted. Here is the relevant code:fn process_sigmask_unblock() { let mut process = Process::with_parent_and_group(Pid(10), Pid(11)); let result = process.block_signals(SigmaskHow::SIG_BLOCK, &[signal::SIGINT, signal::SIGCHLD]); assert_eq!(result, SignalResult::default()); let result = process.block_signals(SigmaskHow::SIG_UNBLOCK, &[signal::SIGINT, signal::SIGQUIT]); assert_eq!(result, SignalResult::default()); let result_set = process.blocked_signals(); assert!(result_set.contains(&signal::SIGCHLD)); assert_eq!(result_set.len(), 1); }This function first blocks
SIGINTandSIGCHLDsignals, then unblocksSIGINTandSIGQUITsignals, and finally checks that onlySIGCHLDremains blocked. The logic appears to correctly handle the unblocking of signals.
- The initial blocking of
SIGINTandSIGCHLDis verified.- The unblocking of
SIGINTandSIGQUITis verified.- The final state ensures that only
SIGCHLDremains blocked.The test accurately reflects the logic for unblocking signals.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `process_sigmask_unblock` test correctly reflects the unblocking logic. # Test: Search for the test usage. Expect: Correct handling of unblocking signals. rg --type rust $'process_sigmask_unblock'Length of output: 114
Script:
#!/bin/bash # Extract the implementation of the `process_sigmask_unblock` function from the specified file. ast-grep --lang rust --pattern 'fn process_sigmask_unblock() { $$$ }' yash-env/src/system/virtual/process.rsLength of output: 1318
459-470: Check theraise_signalmethod for correct signal delivery and state changes.This method is crucial for the correct functioning of signal handling in the virtual system. It needs to correctly handle blocked signals and change the process state as necessary.
Verification successful
The
raise_signalmethod is crucial for handling signals and changing the process state in the virtual system. The search results show multiple usages of this method across different files, indicating its importance.To ensure the method correctly handles signal delivery and state changes, we need to:
- Verify the implementation of the
raise_signalmethod.- Check the context in which this method is used to ensure it is correctly implemented and used.
Let's start by examining the implementation of the
raise_signalmethod in detail.
The
raise_signalmethod inyash-semantics/src/trap/signal.rscalls anotherraise_signalmethod on a process obtained from theVirtualSystem. To verify the correctness of signal handling and state changes, we need to examine theraise_signalmethod inyash-env/src/system/virtual/process.rs.
The previous attempt to retrieve the
raise_signalmethod implementation did not produce any output. To ensure we are targeting the correct file and method, we will usergto search for the method definition inyash-env/src/system/virtual/process.rs.
The
rgcommand confirmed the presence of theraise_signalmethod inyash-env/src/system/virtual/process.rs. To verify its correctness, we need to extract the full implementation of this method.
The
raise_signalmethod inyash-env/src/system/virtual/process.rshas been successfully retrieved. The method handles signal delivery and state changes as follows:
- It sets the process state to
Runningif the signal isSIGCONT.- It checks if the signal is blocked and, if so, adds it to the pending signals.
- It delivers the signal if it is not
SIGKILLorSIGSTOPand not blocked.- It updates the
process_state_changedflag accordingly.This implementation appears to correctly handle signal delivery and state changes.
yash-env/src/system/virtual/process.rs: raise_signalmethod implementation correctly handles signal delivery and state changes.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `raise_signal` method correctly handles the delivery and state changes of signals. # Test: Search for the method usage. Expect: Correct handling of signal delivery and state changes. rg --type rust $'raise_signal'Length of output: 4113
Script:
#!/bin/bash # Description: Retrieve the implementation of the `raise_signal` method to verify its correctness. # Search for the `raise_signal` method implementation in the Rust files. ast-grep --lang rust --pattern 'fn raise_signal($_, $_) { $$$ }'Length of output: 691
Script:
#!/bin/bash # Description: Retrieve the full implementation of the `raise_signal` method in `yash-env/src/system/virtual/process.rs`. # Search for the `raise_signal` method implementation in the specified file. ast-grep --lang rust --pattern 'fn raise_signal(&mut self, signal: signal::Number) -> SignalResult { $$$ }'Length of output: 109
Script:
#!/bin/bash # Description: Retrieve the full implementation of the `raise_signal` method in `yash-env/src/system/virtual/process.rs`. # Search for the `raise_signal` method definition in the specified file. rg 'fn raise_signal' yash-env/src/system/virtual/process.rsLength of output: 138
Script:
#!/bin/bash # Description: Retrieve the full implementation of the `raise_signal` method in `yash-env/src/system/virtual/process.rs`. # Extract the full implementation of the `raise_signal` method. rg -A 20 'fn raise_signal' yash-env/src/system/virtual/process.rsLength of output: 792
yash-env/src/system.rs (1)
241-249: Updatedsigmaskmethod to use a more generic type for signal handling.Ensure all usages of
sigmaskthroughout the codebase are updated to match this new signature.yash-env/src/system/virtual.rs (3)
647-662: The updatedsigmaskfunction correctly implements the new signal handling using a tuple and slice. Good alignment with the PR's objectives.
749-758: The modifications in theselectfunction to use a slice ofsignal::Numberare correctly implemented and consistent with the PR's goal of decoupling from thenixcrate.
Line range hint
2127-2145: Thekillfunction's updates to use the new signal handling structures are correctly implemented. The function maintains its functionality while aligning with the PR's objectives.
| let sigttou = self | ||
| .signal_number_from_name(signal::Name::Ttou) | ||
| .ok_or(Errno::EINVAL)?; | ||
| let mut old_mask = Vec::new(); | ||
|
|
||
| self.sigmask( | ||
| Some((SigmaskHow::SIG_BLOCK, &[sigttou])), | ||
| Some(&mut old_mask), | ||
| )?; | ||
|
|
||
| let result = self.tcsetpgrp(fd, pgid); | ||
|
|
||
| let result_2 = self.sigmask(SigmaskHow::SIG_SETMASK, Some(&old_set), None); | ||
| let result_2 = self.sigmask(Some((SigmaskHow::SIG_SETMASK, &old_mask)), None); |
There was a problem hiding this comment.
Refactored tcsetpgrp_with_block to use the updated sigmask method effectively.
Consider adding unit tests to verify that SIGTTOU is correctly blocked during the operation.
Would you like me to help by adding these tests?
| /// If `signal_mask` is `Some` list of signals, it is used as the signal | ||
| /// blocking mask while waiting and restored when the function returns. | ||
| fn select( | ||
| &mut self, | ||
| readers: &mut FdSet, | ||
| writers: &mut FdSet, | ||
| timeout: Option<&TimeSpec>, | ||
| signal_mask: Option<&SigSet>, | ||
| signal_mask: Option<&[signal::Number]>, |
There was a problem hiding this comment.
Tip
Codebase Verification
The rg command output confirms multiple instances of the select method usage in yash-env/src/system.rs and yash-env/src/system/virtual.rs. These instances need to be updated to match the new method signature.
Here are the specific locations that need attention:
yash-env/src/system.rs:- Line 336:
fn select(&mut self, readers: &mut FdSet, writers: &mut FdSet, timeout: Option<&TimeSpec>, signal_mask: Option<&[signal::Number]>) - Line 343:
(**self.0.borrow_mut()).select(readers, writers, timeout, signal_mask)
- Line 336:
yash-env/src/system/virtual.rs:- Multiple lines where
selectis called with the old signature.
- Multiple lines where
Please ensure these instances are updated to match the new method signature.
Analysis chain
Updated select method to use a more flexible type for the signal mask.
Ensure all usages of select throughout the codebase are updated to match this new signature.
Also applies to: 1056-1056
The rg command output shows multiple instances of select usage across various files. The relevant instances that need to be updated to match the new method signature are primarily in yash-env/src/system.rs and yash-env/src/system/virtual.rs.
Let's verify the specific lines in these files to ensure they match the new method signature.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all function calls to `select` match the new signature.
# Test: Search for the function usage. Expect: Only occurances of the new signature.
rg --type rust $'select'
Length of output: 10202
Script:
#!/bin/bash
# Description: Verify specific lines in `yash-env/src/system.rs` and `yash-env/src/system/virtual.rs` to ensure they match the new method signature.
# Test: Search for the function usage in `yash-env/src/system.rs` and `yash-env/src/system/virtual.rs`. Expect: Only occurrences of the new signature.
rg --type rust 'select' yash-env/src/system.rs yash-env/src/system/virtual.rs
Length of output: 6156
| let Some(sigint) = self.env.system.signal_number_from_name(signal::Name::Int) else { | ||
| return false; | ||
| }; | ||
| let Some(sigquit) = self.env.system.signal_number_from_name(signal::Name::Quit) else { | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
Tip
Codebase Verification
The extracted code snippets show that signal::Name::Int and signal::Name::Quit are used in various parts of the codebase, but there is no explicit handling to ensure that these signal names always map to valid signal numbers. The current implementation relies on the signal_number_from_name function, which returns an Option<signal::Number>, indicating that it might fail.
To address the concern, consider adding error handling or fallback mechanisms to ensure that the signal numbers for SIGINT and SIGQUIT are always retrievable.
yash-env/src/subshell.rs: Lines 277-282yash-env/src/trap.rs: Multiple instancesyash-builtin/src/kill/syntax.rs: Multiple instancesyash-env/src/job/fmt.rs: Line 3
Analysis chain
Handle potential failure to retrieve signal numbers.
Ensure that the system can always retrieve signal numbers for SIGINT and SIGQUIT. If not, consider adding error handling or fallback mechanisms.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that signal numbers are always retrievable.
# Test: Check if there are any cases where signal numbers might not be retrieved.
rg --type rust $'signal_number_from_name'
Length of output: 3096
Script:
#!/bin/bash
# Extract the implementation of the `signal_number_from_name` function to understand its behavior.
# Find the implementation of `signal_number_from_name` in the codebase.
ast-grep --lang rust --pattern 'fn signal_number_from_name(&self, name: signal::Name) -> Option<signal::Number> {
$$$
}'
Length of output: 1292
Script:
#!/bin/bash
# Extract the relevant parts of the implementations of `signal_number_from_name` in `yash-env/src/system/real.rs` and `yash-env/src/system/virtual.rs`.
# Extract the implementation in `yash-env/src/system/real.rs`.
sed -n '354,356p' yash-env/src/system/real.rs
# Extract the implementation in `yash-env/src/system/virtual.rs`.
sed -n '641,643p' yash-env/src/system/virtual.rs
Length of output: 335
Script:
#!/bin/bash
# Extract the relevant parts of the implementations of `to_raw_real` and `to_raw_virtual`.
# Extract the implementation of `to_raw_real`.
ast-grep --lang rust --pattern 'impl signal::Name {
fn to_raw_real(&self) -> Option<signal::Number> {
$$$
}
}'
# Extract the implementation of `to_raw_virtual`.
ast-grep --lang rust --pattern 'impl signal::Name {
fn to_raw_virtual(&self) -> Option<signal::Number> {
$$$
}
}'
Length of output: 244
Script:
#!/bin/bash
# Search for the definitions of `to_raw_real` and `to_raw_virtual` in the codebase.
# Search for `to_raw_real`.
rg 'fn to_raw_real'
# Search for `to_raw_virtual`.
rg 'fn to_raw_virtual'
Length of output: 234
Script:
#!/bin/bash
# Extract the relevant parts of the implementations of `to_raw_real` and `to_raw_virtual`.
# Extract the implementation of `to_raw_real`.
sed -n '/fn to_raw_real/,/}/p' yash-env/src/system/real/signal.rs
# Extract the implementation of `to_raw_virtual`.
sed -n '/fn to_raw_virtual/,/}/p' yash-env/src/system/virtual/signal.rs
Length of output: 707
Script:
#!/bin/bash
# Search for the handling of `signal::Name::Int` and `signal::Name::Quit` in the relevant files.
# Search for `signal::Name::Int` in `yash-env/src/system/real/signal.rs`.
rg 'signal::Name::Int' yash-env/src/system/real/signal.rs
# Search for `signal::Name::Quit` in `yash-env/src/system/real/signal.rs`.
rg 'signal::Name::Quit' yash-env/src/system/real/signal.rs
# Search for `signal::Name::Int` in `yash-env/src/system/virtual/signal.rs`.
rg 'signal::Name::Int' yash-env/src/system/virtual/signal.rs
# Search for `signal::Name::Quit` in `yash-env/src/system/virtual/signal.rs`.
rg 'signal::Name::Quit' yash-env/src/system/virtual/signal.rs
Length of output: 239
Script:
#!/bin/bash
# Search for the handling of `signal::Name::Int` and `signal::Name::Quit` in the entire codebase.
# Search for `signal::Name::Int`.
rg 'signal::Name::Int'
# Search for `signal::Name::Quit`.
rg 'signal::Name::Quit'
Length of output: 1476
Script:
#!/bin/bash
# Extract the relevant parts of the code where `signal::Name::Int` and `signal::Name::Quit` are used.
# Extract the relevant parts in `yash-env/src/subshell.rs`.
sed -n '/signal::Name::Int/,/}/p' yash-env/src/subshell.rs
sed -n '/signal::Name::Quit/,/}/p' yash-env/src/subshell.rs
# Extract the relevant parts in `yash-env/src/trap.rs`.
sed -n '/signal::Name::Int/,/}/p' yash-env/src/trap.rs
sed -n '/signal::Name::Quit/,/}/p' yash-env/src/trap.rs
# Extract the relevant parts in `yash-builtin/src/kill/syntax.rs`.
sed -n '/signal::Name::Int/,/}/p' yash-builtin/src/kill/syntax.rs
sed -n '/signal::Name::Quit/,/}/p' yash-builtin/src/kill/syntax.rs
# Extract the relevant parts in `yash-env/src/job/fmt.rs`.
sed -n '/signal::Name::Quit/,/}/p' yash-env/src/job/fmt.rs
Length of output: 3280
This commit replaces the SigSet type with an array of Signal in the System trait. This change is necessary because the SigSet type is specific to the nix crate, and the System trait should not depend on specific types from external crates.
This commit also updates the implementations of the System trait to reflect this change. SelectSystem::wait_mask is now an optional Vec of Signal, and virtual Process now uses a BTreeSet of Signal instead of SigSet.
Summary by CodeRabbit
Refactor
SigSetwithVec<signal::Number>for better flexibility and performance.New Features
all_signals()function to iterate over all signal numbers.sigset_to_vec()function to convert signal sets to vectors.Bug Fixes