Skip to content
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

vmbus: delay interrupt creation until open response #649

Merged
merged 3 commits into from
Jan 10, 2025

Conversation

jstarks
Copy link
Member

@jstarks jstarks commented Jan 10, 2025

Don't require the guest-to-host interrupt be provided at channel offer time. Wait until open time. This is more flexible, and in the future will allow us to remove some extra synchronziation in vmbus_relay.

Don't require the guest-to-host interrupt be provided at channel offer
time. Wait until open time. This is more flexible, and in the future
will allow us to remove some extra synchronziation in `vmbus_relay`.
@jstarks jstarks requested review from a team as code owners January 10, 2025 20:00
SvenGroot
SvenGroot previously approved these changes Jan 10, 2025
SvenGroot
SvenGroot previously approved these changes Jan 10, 2025
@jstarks jstarks requested a review from Copilot January 10, 2025 20:53
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • vm/devices/vmbus/vmbus_channel/src/bus.rs: Evaluated as low risk
  • vm/devices/vmbus/vmbus_channel/src/channel.rs: Evaluated as low risk
Comments suppressed due to low confidence (5)

vm/devices/vmbus/vmbus_relay/src/lib.rs:425

  • [nitpick] The method name guest_to_host_event is not very descriptive. Consider renaming it to create_guest_to_host_interrupt.
fn guest_to_host_event(&self) -> Interrupt {

vm/devices/vmbus/vmbus_relay/src/lib.rs:367

  • Ensure that the new behavior of delaying the interrupt creation until the open response is covered by tests.
async fn handle_open_channel(&mut self, open_request: &OpenRequest) -> Result<Option<OpenResult>> {

vm/devices/vmbus/vmbus_server/src/lib.rs:665

  • [nitpick] The field name guest_to_host_event is inconsistently named compared to other fields in the struct. Consider renaming it to match the naming convention used for other fields.
guest_to_host_event: Arc<ChannelEvent>,

vm/devices/vmbus/vmbus_server/src/lib.rs:1397

  • The complete_open function introduces new behavior that is not covered by tests. Consider adding tests to cover this new behavior.
fn complete_open(&mut self, offer_id: OfferId, result: Option<OpenResult>) -> &mut Channel {

vm/devices/net/netvsp/src/test.rs:498

  • The variable guest_to_host_interrupt is declared as None and might not be assigned a value if the rpc.handle_sync closure is not executed. Ensure that guest_to_host_interrupt is properly assigned a value.
let mut guest_to_host_interrupt = None;

vm/devices/vmbus/vmbus_channel/src/offer.rs Outdated Show resolved Hide resolved
@jstarks jstarks merged commit ec7cfcb into microsoft:main Jan 10, 2025
25 checks passed
@jstarks jstarks deleted the event_at_open branch January 10, 2025 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants