qemu: Add async QMP client, always-on QMP monitor, and NetworkMode::None#243
qemu: Add async QMP client, always-on QMP monitor, and NetworkMode::None#243cgwalters wants to merge 1 commit intobootc-dev:mainfrom
Conversation
QMP is useful for many things around dynamic control of the VM. Let's add a QMP channel by default. This adds some basic infrastructure around hotplugging virtio-serial channels, which I'm thinking about using for dynamic host <-> VM communications. Also add NetworkMode::None for fully isolated VMs where all communication happens over virtio-serial. Assisted-by: OpenCode (Claude Opus 4) Signed-off-by: Colin Walters <walters@verbum.org>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a QMP (QEMU Machine Protocol) client and support for bidirectional virtio-serial devices using Unix sockets, enabling runtime VM control such as hot-plugging ports. Key additions include the QmpClient implementation, a new VirtioSerialBidir device type, and a NetworkMode::None option for isolated VMs. Several critical issues were identified: the hardcoded global QMP socket path prevents concurrent VM instances, a race condition in the QMP client could lead to lost events, the hot-unplug logic lacks necessary event filtering and timeouts, and there is a potential command injection vulnerability if socket paths contain commas.
| pub const VHOST_VSOCK: &str = "/dev/vhost-vsock"; | ||
|
|
||
| /// Default path for the QMP (QEMU Machine Protocol) Unix socket. | ||
| pub const QMP_SOCKET_PATH: &str = "/run/bcvk-qmp.sock"; |
There was a problem hiding this comment.
The default QMP socket path is hardcoded to a single global location (/run/bcvk-qmp.sock). This will cause conflicts and permission issues if multiple VMs are started simultaneously on the same host. Consider using a unique path per VM instance (e.g., by generating a path in a temporary directory or requiring the caller to provide a unique path).
| if let Some(event) = response.get("event") { | ||
| debug!("QMP event (while waiting for response): {}", event); | ||
| } |
There was a problem hiding this comment.
Events received while waiting for a command response are logged and discarded. This creates a race condition: if a DEVICE_DELETED event arrives immediately after a device_del command is processed but before the response is read, it will be lost. A subsequent call to wait_for_event will then hang indefinitely. A robust implementation should buffer events in an internal queue to ensure they are not missed during command execution.
| self.wait_for_event("DEVICE_DELETED") | ||
| .await | ||
| .with_context(|| format!("waiting for {dev_id} to be removed"))?; |
There was a problem hiding this comment.
This wait loop is missing event filtering and a timeout. It currently returns on the first DEVICE_DELETED event seen, which might belong to a different device if multiple unplugs are occurring. It also lacks the 5-second timeout mentioned in the docstring, which could lead to indefinite hangs if the guest fails to acknowledge the removal.
| self.wait_for_event("DEVICE_DELETED") | |
| .await | |
| .with_context(|| format!("waiting for {dev_id} to be removed"))?; | |
| tokio::time::timeout(std::time::Duration::from_secs(5), async { | |
| loop { | |
| let event = self.wait_for_event("DEVICE_DELETED").await?; | |
| if event["data"]["device"] == dev_id { | |
| return Ok(()); | |
| } | |
| } | |
| }) | |
| .await | |
| .map_err(|e| eyre!("Timeout waiting for device removal: {e}"))??; |
| &format!( | ||
| "socket,id={},path={},server=on,wait=off", | ||
| char_id, bidir_device.socket_path | ||
| ), |
There was a problem hiding this comment.
QEMU uses commas as delimiters for -chardev options. If bidir_device.socket_path contains a comma, it will break the command line parsing and potentially allow for argument injection. QEMU allows escaping commas by doubling them (,,).
| &format!( | |
| "socket,id={},path={},server=on,wait=off", | |
| char_id, bidir_device.socket_path | |
| ), | |
| &format!( | |
| "socket,id={},path={},server=on,wait=off", | |
| char_id, bidir_device.socket_path.replace(',', ",,") | |
| ), |
1 participant
QMP is useful for many things around dynamic control of the VM. Let's add a QMP channel by default. This adds some basic infrastructure around hotplugging virtio-serial channels, which I'm thinking about using for dynamic host <-> VM communications.
Also add NetworkMode::None for fully isolated VMs where all communication happens over virtio-serial.
Assisted-by: OpenCode (Claude Opus 4)