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

Jump to bottom

Pop pending events after pausing vm #1026

Merged
merged 3 commits into from Jun 2, 2022
Merged

Pop pending events after pausing vm #1026

merged 3 commits into from Jun 2, 2022

Conversation

rageagainsthepc
Copy link
Contributor

In some edge cases the pause event might not be the first event in the queue. This causes kvm_resume_vm to fail. Therefore we should process all pending events immediately after pausing the vm and let the pause event be inserted into pause_events_list.

@rageagainsthepc
Copy link
Contributor Author

I haven't tested these changes yet, therefore the draft mode.

@rageagainsthepc rageagainsthepc force-pushed the vm-resume-edge-case branch 4 times, most recently from 0dce317 to f96bf70 Compare May 9, 2022 15:50
@rageagainsthepc
Copy link
Contributor Author

Apparently it's not possible to process pause events immediately in kvm_pause_vm, but we should at least process all events right before resuming so we can be sure that all pause events are waiting in pause_events_list.

@rageagainsthepc rageagainsthepc marked this pull request as ready for review May 9, 2022 15:54
@rageagainsthepc rageagainsthepc force-pushed the vm-resume-edge-case branch 2 times, most recently from 62e0939 to 599c6ac Compare May 9, 2022 15:58
@tklengyel
Copy link
Contributor

/cc @Wenzel

@rageagainsthepc
Copy link
Contributor Author

Fyi, I've just discovered that there is still a bug in the implementation.

@Wenzel
Copy link
Member

Wenzel commented May 18, 2022

@rageagainsthepc can you describe the bug you found ?

@rageagainsthepc
Copy link
Contributor Author

@Wenzel Sure. My assumption that all pause events have been sent by kvm by the time we call kvm_resume_vm was apparently incorrect. I assume that's why you wait for new events until you reach expected_pause_count. I am going to rework this PR soon.

@rageagainsthepc rageagainsthepc force-pushed the vm-resume-edge-case branch 3 times, most recently from e52003e to 85c501b Compare May 18, 2022 14:12
@rageagainsthepc rageagainsthepc force-pushed the vm-resume-edge-case branch 2 times, most recently from 11a56e6 to a2831e1 Compare May 18, 2022 14:53
@rageagainsthepc
Copy link
Contributor Author

It seems to work for me now. I guess this is ready for review again. 👍

@Wenzel
Copy link
Member

Wenzel commented May 21, 2022

LGTM on the review, but I haven't tested the PR.

@rageagainsthepc
Copy link
Contributor Author

What about this PR? Is there something left to be done?

@tklengyel tklengyel merged commit 50d256b into libvmi:master Jun 2, 2022
@rageagainsthepc rageagainsthepc deleted the vm-resume-edge-case branch June 2, 2022 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wenzel Wenzel Wenzel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rageagainsthepc @tklengyel @Wenzel