-
Notifications
You must be signed in to change notification settings - Fork 170
Improve InterruptHandle::kill() documentation and clarify boolean return value #1121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
f936bc0
c34ddfa
c8e471e
5350a8c
ad4591d
0daa0fd
21cdb54
28890e7
8b7a60c
533f3a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,7 +166,6 @@ sequenceDiagram | |
| end | ||
|
|
||
| deactivate IH | ||
| IH-->>Caller: sent_signal | ||
| deactivate IH | ||
| ``` | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -208,12 +208,19 @@ pub(crate) trait InterruptHandleImpl: InterruptHandle { | |
| pub trait InterruptHandle: Send + Sync + Debug { | ||
| /// Interrupt the corresponding sandbox from running. | ||
| /// | ||
| /// - If this is called while the the sandbox currently executing a guest function call, it will interrupt the sandbox and return `true`. | ||
| /// - If this is called while the sandbox is not running (for example before or after calling a guest function), it will do nothing and return `false`. | ||
| /// This method sets a cancellation flag that prevents or stops the execution of guest code. | ||
| /// The effectiveness of this call depends on timing relative to the guest function call lifecycle: | ||
| /// | ||
| /// - **Before guest call starts** (before `clear_cancel()` in `MultiUseSandbox::call()`): | ||
| /// The cancellation request will be cleared and ignored. | ||
| /// - **After guest call starts but before entering guest code** (after `clear_cancel()`, before `run_vcpu()`): | ||
| /// Will prevent the guest from executing. | ||
| /// - **While executing guest code**: Will interrupt the vCPU. | ||
| /// - **After guest call completes**: Has no effect (cancellation is cleared at the start of the next call). | ||
|
jsturtevant marked this conversation as resolved.
Outdated
|
||
| /// | ||
| /// # Note | ||
| /// This function will block for the duration of the time it takes for the vcpu thread to be interrupted. | ||
| fn kill(&self) -> bool; | ||
| fn kill(&self); | ||
|
|
||
| /// Used by a debugger to interrupt the corresponding sandbox from running. | ||
| /// | ||
|
|
@@ -374,13 +381,13 @@ impl InterruptHandleImpl for LinuxInterruptHandle { | |
|
|
||
| #[cfg(any(kvm, mshv3))] | ||
| impl InterruptHandle for LinuxInterruptHandle { | ||
| fn kill(&self) -> bool { | ||
| fn kill(&self) { | ||
| // Release ordering ensures that any writes before kill() are visible to the vcpu thread | ||
| // when it checks is_cancelled() with Acquire ordering | ||
| self.state.fetch_or(Self::CANCEL_BIT, Ordering::Release); | ||
|
|
||
| // Send signals to interrupt the vcpu if it's currently running | ||
| self.send_signal() | ||
| self.send_signal(); | ||
| } | ||
|
|
||
| #[cfg(gdb)] | ||
|
|
@@ -513,7 +520,7 @@ impl InterruptHandleImpl for WindowsInterruptHandle { | |
|
|
||
| #[cfg(target_os = "windows")] | ||
| impl InterruptHandle for WindowsInterruptHandle { | ||
| fn kill(&self) -> bool { | ||
| fn kill(&self) { | ||
| use windows::Win32::System::Hypervisor::WHvCancelRunVirtualProcessor; | ||
|
|
||
| // Release ordering ensures that any writes before kill() are visible to the vcpu thread | ||
|
|
@@ -524,7 +531,7 @@ impl InterruptHandle for WindowsInterruptHandle { | |
| // This ensures we see the running state set by the vcpu thread | ||
| let state = self.state.load(Ordering::Acquire); | ||
| if state & Self::RUNNING_BIT == 0 { | ||
| return false; | ||
| return; | ||
| } | ||
|
|
||
| // Take read lock to prevent race with WHvDeletePartition in set_dropped(). | ||
|
|
@@ -534,15 +541,19 @@ impl InterruptHandle for WindowsInterruptHandle { | |
| Ok(guard) => guard, | ||
| Err(e) => { | ||
| log::error!("Failed to acquire partition_state read lock: {}", e); | ||
| return false; | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| if guard.dropped { | ||
| return false; | ||
| return; | ||
| } | ||
|
|
||
| unsafe { WHvCancelRunVirtualProcessor(guard.handle, 0, 0).is_ok() } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot Lets skip this, we have logging in other places
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the error logging for WHvCancelRunVirtualProcessor in commit 8b7a60c. The method now simply returns |
||
| unsafe { | ||
| if let Err(e) = WHvCancelRunVirtualProcessor(guard.handle, 0, 0) { | ||
| log::error!("Failed to cancel running virtual processor: {}", e); | ||
| } | ||
| }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ludfjig it seems the boolean here was useful information?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes but it would return false when cancelling before vcpu started executing.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am leaning towards keeping the bool and changing the wording in the docs since the linux implementation also returned a boolean before this. I am not a big fan of this change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm open to that idea too.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot Lets keep the bool and update the comment to match the usage, specifically that it does not mean that it was success but that the signal was sent on linux and on windows it means true means it was killed but false could mean other things
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Restored the boolean return value and updated the documentation in commit 0daa0fd. The docs now clarify that:
All test assertions have been restored. |
||
| } | ||
| #[cfg(gdb)] | ||
| fn kill_from_debugger(&self) -> bool { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot don't make these changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted Cargo.toml changes in commit 28890e7. The tracing-tracy dependency is now back to being a regular dev-dependency as it was originally.