-
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 3 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,13 @@ 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: | ||
| /// - If called while the sandbox is currently executing a guest function, it will interrupt the vCPU. | ||
| /// - If called before the sandbox starts executing (e.g., before a guest function call), it will prevent execution from starting. | ||
| /// | ||
| /// # 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 +375,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 +514,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 +525,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 +535,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 This sentence is not entirely accurate. If called before a guest call, indeed the kill() is ignored. But if called after guest call, but before entering the guest, it is still effective. Please analyze the code and update the docs to accurately describe the behavior of kill() to make it easier for readers to understand what kill() does
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.
Updated the documentation to accurately describe the timing behavior. The key clarification is that
kill()is only ignored if called beforeclear_cancel()in the guest function call lifecycle. If called after that point but before entering guest code, it will still prevent execution. See commit 5350a8c.