Implement OpenVMM backend - #41697
Daman Mulye (damanm24) wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical cancellation and VM-lifetime races, along with additional networking and validation issues, block approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (5)
What changed in this PR
Implements the remaining IVirtualMachineBackend functionality for OpenVMM, including filesystem shares and networking.
Changes:
- Adds filesystem share and device lifecycle management.
- Adds network adapters, port binding, and guest connections.
- Expands lifecycle, cancellation, and integration tests.
| File | Summary and findings |
|---|---|
test/windows/OpenVmmVirtualMachineBackendTests.cpp |
Adds filesystem, networking, and cancellation tests. Nit (2 votes): bind coverage does not verify end-to-end traffic. Moderate (2 votes): cancellation test synchronization can race guest-accept startup. |
src/windows/common/OpenVmmVirtualMachineBackend.h |
Adds backend resource state and APIs. |
src/windows/common/OpenVmmVirtualMachineBackend.cpp |
Implements filesystem, networking, listener, and lifecycle operations. Critical (3 votes): cancellation remains permanently signaled. Critical (1 vote): m_vm access can race termination. Moderate (4 votes): out-of-range disk LUN validation is unreachable. Moderate (1 vote each): listener and client sockets require overlapped AF_UNIX creation; requested network configuration is not fully applied. |
src/windows/common/IVirtualMachineBackend.h |
Extends VM descriptions and backend request types. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| std::filesystem::path GetVsockListenerPath(const std::filesystem::path& VsockPath, GuestServicePort Port) | ||
| { | ||
| return std::format(L"{}_{:08x}-facb-11e6-bd58-64006a7986d3", VsockPath.native(), Port.Value); |
There was a problem hiding this comment.
What's the context for this constant guid ? Is that an openvmm value ?
There was a problem hiding this comment.
Yeah this comes from OpenVMM's hybrid_vsock implementation which bridges hvsocket and UDS. See: https://github.com/SvenGroot/openvmm/blob/main/support/hybrid_vsock/src/lib.rs. This GUID is an embedding of the AF_VSOCK port into an AF_HYPERV service ID.
| THROW_WIN32_IF( | ||
| static_cast<DWORD>(WSAGetLastError()), connect(socket.get(), reinterpret_cast<const sockaddr*>(&address), sizeof(address)) == SOCKET_ERROR); | ||
|
|
||
| const auto request = std::format("CONNECT {}\n", Port.Value); |
There was a problem hiding this comment.
For my own knowledge, is this "CONNECT" request going to be interpreted by openvmm and then connect into the right hvsocket port on the guest, or is the expectation that we'll have a linux usermode process doing that for ourselves ?
There was a problem hiding this comment.
Yes, the former interpretation is correct. CONNECT is interpreted by OpenVMM and the hvsocket service ID which follows the CONNECT string contains the AF_VSOCK port number.
This PR implements the rest of the
IVirtualMachineBackendinterface for theOpenVmmVirtualMachineBackendclass. Specifically, it adds support for: filesystem shares and networking devices.