From 07693a6aec18bd729e1086b12a6459d83a84d542 Mon Sep 17 00:00:00 2001 From: Stephen Akinyemi Date: Thu, 23 Apr 2026 23:12:46 +0100 Subject: [PATCH 1/3] feat(krun): expand DiskBuilder with id, cache, direct_io, sync Surface the per-disk knobs that libkrun's krun_add_disk3 C API already supports but the Rust wrapper had hardcoded. This lets callers attach extra disks as volumes (not just a rootfs) with stable identifiers and per-disk safety/perf settings. - Add CacheMode { Writeback, Unsafe } and SyncMode { None, Relaxed, Full } enums with From impls to devices::virtio::{CacheType, block::SyncMode}. - Add DiskBuilder::id() for caller-chosen block_ids, enabling guest addressing via /dev/disk/by-id/virtio- independent of attach order. - Add DiskBuilder::cache(), direct_io(), sync() alongside the existing path/format/read_only methods. - Replace the naive format!("vd{}", (b'a' + i) as char) device-id fallback, which rolled over into invalid characters past vdz, with a bijective base-26 vd_suffix() helper matching block/genhd.c's disk_name() scheme (vda..vdz, vdaa..vdzz, vdaaa...). - Re-export CacheMode and SyncMode from the crate root. Defaults (Writeback cache, direct_io=false, Full sync) preserve the previous hardcoded behavior, so existing .disk(|d| d.path().format() .read_only()) callers are unaffected. --- src/krun/src/api/builder.rs | 126 +++++++++++++++++++++++++++++++++-- src/krun/src/api/builders.rs | 100 +++++++++++++++++++++++++++ src/krun/src/lib.rs | 4 ++ 3 files changed, 224 insertions(+), 6 deletions(-) diff --git a/src/krun/src/api/builder.rs b/src/krun/src/api/builder.rs index 8567b8a72..8aa4dc72e 100644 --- a/src/krun/src/api/builder.rs +++ b/src/krun/src/api/builder.rs @@ -210,15 +210,30 @@ impl VmBuilder { /// Configure block devices. /// - /// Can be called multiple times to add multiple devices. + /// Can be called multiple times to add multiple devices. Devices receive + /// deterministic guest names by attach order (`/dev/vda`, `/dev/vdb`, + /// ...). For stable addressing across reorderings, set a custom `id()` — + /// the guest can then reach the disk via `/dev/disk/by-id/virtio-`. /// - /// # Example + /// # Examples + /// + /// Single rootfs disk: /// /// ```rust,no_run /// # use msb_krun::VmBuilder; /// VmBuilder::new() /// .disk(|d| d.path("/path/to/disk.img").read_only(true)); /// ``` + /// + /// Rootfs plus data and cache volumes with stable ids: + /// + /// ```rust,no_run + /// # use msb_krun::{VmBuilder, DiskImageFormat}; + /// VmBuilder::new() + /// .disk(|d| d.path("/img/root.raw")) + /// .disk(|d| d.path("/img/data.qcow2").format(DiskImageFormat::Qcow2).id("data")) + /// .disk(|d| d.path("/img/cache.raw").id("cache").read_only(true)); + /// ``` #[cfg(feature = "blk")] pub fn disk(mut self, f: impl FnOnce(DiskBuilder) -> DiskBuilder) -> Self { let new_disk = f(DiskBuilder::new()).finalize(); @@ -437,17 +452,22 @@ impl VmBuilder { // Apply block device configuration #[cfg(feature = "blk")] for (i, config) in self.disk.configs.into_iter().enumerate() { - let block_id = format!("vd{}", (b'a' + i as u8) as char); + let block_id = config + .id + .clone() + .unwrap_or_else(|| format!("vd{}", vd_suffix(i))); let image_type: ImageType = config.format.into(); + let cache_type: CacheType = config.cache.into(); + let sync_mode: devices::virtio::block::SyncMode = config.sync.into(); let blk_config = BlockDeviceConfig { block_id, - cache_type: CacheType::Writeback, + cache_type, disk_image_path: config.path.to_string_lossy().to_string(), disk_image_format: image_type, is_disk_read_only: config.read_only, - direct_io: false, - sync_mode: devices::virtio::block::SyncMode::default(), + direct_io: config.direct_io, + sync_mode, }; vmr.add_block_device(blk_config) @@ -543,6 +563,27 @@ fn generate_mac(index: usize) -> [u8; 6] { ] } +/// Generate a virtio-blk device suffix matching the Linux kernel's +/// `disk_name()` bijective base-26 scheme from `block/genhd.c`: +/// `0→"a"`, `25→"z"`, `26→"aa"`, `27→"ab"`, `701→"zz"`, `702→"aaa"`. +/// +/// The naïve `(b'a' + i) as char` formula rolls over past `z` into +/// invalid characters (`vd{`, `vd|`, ...), so this helper is required +/// once we support more than 26 disks. +#[cfg(feature = "blk")] +fn vd_suffix(mut index: usize) -> String { + let mut buf = Vec::with_capacity(4); + loop { + buf.push(b'a' + (index % 26) as u8); + if index < 26 { + break; + } + index = index / 26 - 1; + } + buf.reverse(); + String::from_utf8(buf).expect("ASCII a-z only") +} + fn map_vm_config_error(machine: &MachineBuilder, err: VmConfigError) -> Error { match err { VmConfigError::InvalidVcpuCount => { @@ -577,4 +618,77 @@ mod tests { other => panic!("unexpected error: {other:?}"), } } + + #[cfg(feature = "blk")] + #[test] + fn vd_suffix_matches_kernel_scheme() { + assert_eq!(vd_suffix(0), "a"); + assert_eq!(vd_suffix(25), "z"); + assert_eq!(vd_suffix(26), "aa"); + assert_eq!(vd_suffix(27), "ab"); + assert_eq!(vd_suffix(51), "az"); + assert_eq!(vd_suffix(52), "ba"); + assert_eq!(vd_suffix(701), "zz"); + assert_eq!(vd_suffix(702), "aaa"); + } + + #[cfg(feature = "blk")] + #[test] + fn disk_builder_preserves_insertion_order() { + use crate::api::builders::DiskImageFormat; + + let builder = VmBuilder::new() + .disk(|d| d.path("/a.raw")) + .disk(|d| d.path("/b.qcow2").format(DiskImageFormat::Qcow2)) + .disk(|d| d.path("/c.vmdk").format(DiskImageFormat::Vmdk).read_only(true)); + + let paths: Vec<_> = builder + .disk + .configs + .iter() + .map(|c| c.path.to_string_lossy().into_owned()) + .collect(); + assert_eq!(paths, vec!["/a.raw", "/b.qcow2", "/c.vmdk"]); + assert_eq!(builder.disk.configs[1].format, DiskImageFormat::Qcow2); + assert!(builder.disk.configs[2].read_only); + } + + #[cfg(feature = "blk")] + #[test] + fn disk_builder_auto_id_then_custom() { + let builder = VmBuilder::new() + .disk(|d| d.path("/a.raw")) + .disk(|d| d.path("/b.raw").id("data")) + .disk(|d| d.path("/c.raw")); + + assert!(builder.disk.configs[0].id.is_none()); + assert_eq!(builder.disk.configs[1].id.as_deref(), Some("data")); + assert!(builder.disk.configs[2].id.is_none()); + } + + #[cfg(feature = "blk")] + #[test] + fn disk_builder_per_disk_settings_dont_leak() { + use crate::api::builders::{CacheMode, SyncMode}; + + let builder = VmBuilder::new() + .disk(|d| { + d.path("/a.raw") + .read_only(true) + .cache(CacheMode::Unsafe) + .direct_io(true) + .sync(SyncMode::None) + }) + .disk(|d| d.path("/b.raw")); + + assert!(builder.disk.configs[0].read_only); + assert_eq!(builder.disk.configs[0].cache, CacheMode::Unsafe); + assert!(builder.disk.configs[0].direct_io); + assert_eq!(builder.disk.configs[0].sync, SyncMode::None); + + assert!(!builder.disk.configs[1].read_only); + assert_eq!(builder.disk.configs[1].cache, CacheMode::Writeback); + assert!(!builder.disk.configs[1].direct_io); + assert_eq!(builder.disk.configs[1].sync, SyncMode::Full); + } } diff --git a/src/krun/src/api/builders.rs b/src/krun/src/api/builders.rs index ad00904d1..76db4e0c4 100644 --- a/src/krun/src/api/builders.rs +++ b/src/krun/src/api/builders.rs @@ -259,6 +259,30 @@ pub enum DiskImageFormat { Vmdk, } +/// Host-side cache behavior for a block device. +/// +/// Mirrors libkrun's internal `CacheType`. `Writeback` honors guest flush +/// requests via `fsync`; `Unsafe` advertises flush but makes it a noop. +#[cfg(feature = "blk")] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CacheMode { + Writeback, + Unsafe, +} + +/// Host-side sync behavior for a block device. +/// +/// `None` skips all host sync (fastest, data-loss risk). `Relaxed` honors +/// `VIRTIO_BLK_F_FLUSH` but relaxes hardware sync. `Full` forces strict +/// hardware flush. +#[cfg(feature = "blk")] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SyncMode { + None, + Relaxed, + Full, +} + /// Builder for block device configuration. /// /// # Example @@ -273,8 +297,12 @@ pub enum DiskImageFormat { pub struct DiskBuilder { pub(crate) configs: Vec, current_path: Option, + current_id: Option, current_read_only: bool, current_format: DiskImageFormat, + current_cache: CacheMode, + current_direct_io: bool, + current_sync: SyncMode, } /// Configuration for a single block device. @@ -282,8 +310,14 @@ pub struct DiskBuilder { #[derive(Debug, Clone)] pub struct DiskConfig { pub path: PathBuf, + /// Caller-chosen block id. When `None`, the builder auto-assigns + /// `vd` using the kernel's bijective base-26 scheme. + pub id: Option, pub read_only: bool, pub format: DiskImageFormat, + pub cache: CacheMode, + pub direct_io: bool, + pub sync: SyncMode, } //-------------------------------------------------------------------------------------------------- @@ -722,8 +756,12 @@ impl DiskBuilder { Self { configs: Vec::new(), current_path: None, + current_id: None, current_read_only: false, current_format: DiskImageFormat::Raw, + current_cache: CacheMode::Writeback, + current_direct_io: false, + current_sync: SyncMode::Full, } } @@ -733,17 +771,36 @@ impl DiskBuilder { self } + /// Set a stable block id for the current disk. + /// + /// When unset, the builder auto-assigns `vd` by insertion order + /// (matching the kernel's bijective base-26 scheme: `vda`..`vdz`, + /// `vdaa`..`vdzz`, `vdaaa`...). Setting a custom id lets the guest + /// address the device via `/dev/disk/by-id/virtio-` independent of + /// attach order. + pub fn id(mut self, id: impl Into) -> Self { + self.current_id = Some(id.into()); + self + } + /// Set the path for a block device. pub fn path(mut self, path: impl AsRef) -> Self { // Finalize any pending config if let Some(pending_path) = self.current_path.take() { self.configs.push(DiskConfig { path: pending_path, + id: self.current_id.take(), read_only: self.current_read_only, format: self.current_format, + cache: self.current_cache, + direct_io: self.current_direct_io, + sync: self.current_sync, }); self.current_read_only = false; self.current_format = DiskImageFormat::Raw; + self.current_cache = CacheMode::Writeback; + self.current_direct_io = false; + self.current_sync = SyncMode::Full; } self.current_path = Some(path.as_ref().to_path_buf()); @@ -756,13 +813,35 @@ impl DiskBuilder { self } + /// Set the host-side cache behavior for the current disk. + pub fn cache(mut self, mode: CacheMode) -> Self { + self.current_cache = mode; + self + } + + /// Enable or disable `O_DIRECT` on the host backing file for the current disk. + pub fn direct_io(mut self, enabled: bool) -> Self { + self.current_direct_io = enabled; + self + } + + /// Set the host-side sync behavior for the current disk. + pub fn sync(mut self, mode: SyncMode) -> Self { + self.current_sync = mode; + self + } + /// Finalize the builder (called internally). pub(crate) fn finalize(mut self) -> Self { if let Some(path) = self.current_path.take() { self.configs.push(DiskConfig { path, + id: self.current_id.take(), read_only: self.current_read_only, format: self.current_format, + cache: self.current_cache, + direct_io: self.current_direct_io, + sync: self.current_sync, }); } self @@ -790,3 +869,24 @@ impl From for devices::virtio::block::ImageType { } } } + +#[cfg(feature = "blk")] +impl From for devices::virtio::CacheType { + fn from(mode: CacheMode) -> Self { + match mode { + CacheMode::Writeback => devices::virtio::CacheType::Writeback, + CacheMode::Unsafe => devices::virtio::CacheType::Unsafe, + } + } +} + +#[cfg(feature = "blk")] +impl From for devices::virtio::block::SyncMode { + fn from(mode: SyncMode) -> Self { + match mode { + SyncMode::None => devices::virtio::block::SyncMode::None, + SyncMode::Relaxed => devices::virtio::block::SyncMode::Relaxed, + SyncMode::Full => devices::virtio::block::SyncMode::Full, + } + } +} diff --git a/src/krun/src/lib.rs b/src/krun/src/lib.rs index e7adc60d4..52a92ed3a 100644 --- a/src/krun/src/lib.rs +++ b/src/krun/src/lib.rs @@ -39,9 +39,13 @@ pub mod backends; pub use api::builder::VmBuilder; #[cfg(feature = "blk")] +pub use api::builders::CacheMode; +#[cfg(feature = "blk")] pub use api::builders::DiskBuilder; #[cfg(feature = "blk")] pub use api::builders::DiskImageFormat; +#[cfg(feature = "blk")] +pub use api::builders::SyncMode; #[cfg(feature = "net")] pub use api::builders::NetBuilder; pub use api::builders::{ConsoleBuilder, ExecBuilder, FsBuilder, KernelBuilder, MachineBuilder}; From 17768bb607fb49adca4317885f6b9d18d63b172f Mon Sep 17 00:00:00 2001 From: Stephen Akinyemi Date: Thu, 23 Apr 2026 23:23:27 +0100 Subject: [PATCH 2/3] style(krun): apply cargo fmt --- src/krun/src/api/builder.rs | 6 +++++- src/krun/src/lib.rs | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/krun/src/api/builder.rs b/src/krun/src/api/builder.rs index 8aa4dc72e..5672ec62d 100644 --- a/src/krun/src/api/builder.rs +++ b/src/krun/src/api/builder.rs @@ -640,7 +640,11 @@ mod tests { let builder = VmBuilder::new() .disk(|d| d.path("/a.raw")) .disk(|d| d.path("/b.qcow2").format(DiskImageFormat::Qcow2)) - .disk(|d| d.path("/c.vmdk").format(DiskImageFormat::Vmdk).read_only(true)); + .disk(|d| { + d.path("/c.vmdk") + .format(DiskImageFormat::Vmdk) + .read_only(true) + }); let paths: Vec<_> = builder .disk diff --git a/src/krun/src/lib.rs b/src/krun/src/lib.rs index 52a92ed3a..e7baac2ba 100644 --- a/src/krun/src/lib.rs +++ b/src/krun/src/lib.rs @@ -44,10 +44,10 @@ pub use api::builders::CacheMode; pub use api::builders::DiskBuilder; #[cfg(feature = "blk")] pub use api::builders::DiskImageFormat; -#[cfg(feature = "blk")] -pub use api::builders::SyncMode; #[cfg(feature = "net")] pub use api::builders::NetBuilder; +#[cfg(feature = "blk")] +pub use api::builders::SyncMode; pub use api::builders::{ConsoleBuilder, ExecBuilder, FsBuilder, KernelBuilder, MachineBuilder}; pub use api::error::{BuildError, ConfigError, Error, Result, RuntimeError}; pub use api::exit_handle::ExitHandle; From 6623058ba7d37a4d2c5432629ca7fc2ed0c6203c Mon Sep 17 00:00:00 2001 From: Stephen Akinyemi Date: Sun, 26 Apr 2026 19:00:40 +0100 Subject: [PATCH 3/3] fix(block): propagate caller-supplied id to virtio-blk disk_image_id When the caller passes a non-empty id via DiskBuilder::id() it now shows up in the guest as the virtio-blk serial (visible at /sys/block//serial and, with udev, /dev/disk/by-id/virtio-). Previously the id was stored as Block.id but disk_image_id was always generated from the host file's st_rdev + st_ino, so the configured id never reached the guest. Falls back to the rdev+inode default when id is empty, preserving existing behaviour for callers that do not set an id. Padding/truncation matches the existing default path: zero-padded to VIRTIO_BLK_ID_BYTES (20), with longer ids truncated. --- src/devices/src/virtio/block/device.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs index c943b10e0..94fe6f9ef 100644 --- a/src/devices/src/virtio/block/device.rs +++ b/src/devices/src/virtio/block/device.rs @@ -243,7 +243,19 @@ impl Block { .write(!is_disk_read_only) .open(PathBuf::from(&disk_image_path))?; - let disk_image_id = DiskProperties::build_disk_image_id(&disk_image); + // Use the caller-supplied `id` as the virtio-blk disk image id so + // it surfaces in the guest at `/sys/block//serial` and (when + // udev is available) `/dev/disk/by-id/virtio-`. Falls back to + // the rdev+inode-derived default if `id` is empty. + let disk_image_id = if id.is_empty() { + DiskProperties::build_disk_image_id(&disk_image) + } else { + let mut padded = vec![0u8; VIRTIO_BLK_ID_BYTES as usize]; + let bytes = id.as_bytes(); + let n = cmp::min(bytes.len(), padded.len()); + padded[..n].copy_from_slice(&bytes[..n]); + padded + }; let file_opts = StorageOpenOptions::new() .write(!is_disk_read_only)