This repository has been archived by the owner on Sep 7, 2021. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implementation of vCPU crate #3
base: main
Are you sure you want to change the base?
Implementation of vCPU crate #3
Changes from all commits
8c6b98a
8f775bd
ec9817b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Just noticed that. We will have a problem here because rust doesn't allow cyclic dependencies between crates. Isn't the idea to have
kvm-ioctls
importvmm-vcpu
as well?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.
Good point about the circular dependency. This dependency isn't strictly necessary; it is only needed for the CpuId structure doc comment examples, which are run as tests when running
cargo test
(thusdev-dependencies
being a dependency for tests only).Those tests/examples will become outdated anyway with the upcoming crate for "KvmVec"-like generic abstractions (and CpuId and MsrEntries implementations), ie here. But when that change is incorporated, the same problem will remain: What is the best way to handle platform-specific doc comment examples in abstraction crates. This looks to be a common problem with a solution that the rust developers agree should be allowed. But seeing as that issue is still open, I see three options, none perfect:
dev-dependencies
when publishing?)ignore
directive so that they are visible as documentation, but not actually run withcargo test
.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.
I would suggest to export the structures uniformly on both platforms. Instead of exporting them as mod x86_64 and mod arm, you can export the structures directly.
So basically when you want to use some structures from this crate in an external crate you would write:
use vmm_vcpu::StandardRegisters
instead ofuse vmm_vcpu::x86_64::StandardRegisters
.My thinking here is that x86 and arm are not logical modules, they're just a way for us to separate the code into modules so we don't have a huge file with all defines. What do you think?
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.
I am having second doubts about it. Before doing any chance I would try to see how other multi-platform crates are doing this.
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.
I went through the same though process having read your comments. On reading your first comment, I was in complete agreement that exporting the structures was cleaner. But having thought it over, I am now leaning toward keeping them as-is (exporting as
mod x86_64
andmod arm
). The reason being that it forces the code to be explicit that the structures come from specific architectures. This seems particularly important given that the structures can have generic names (SpecialRegisters
) but represent architecture-dependent structures. Having scanned through various projects on github, I'm not seeing much that exactly fit this use/hierarchy, and so are not directly comparable, but it does seem that exporting the module instead of the structures is more common.Let me know if you have strong feelings either way, having thought about it over a couple days.