feat(arch): rust-vmm arch #1
Conversation
WalkthroughRestructures the project: removes docs, tests, embedded OCI LFS pointers, and legacy VM runner; converts Rust crate to a cdylib with a rust-vmm scaffold; introduces a typed PyO3 API (prepare_image, run) and new VMM modules; updates packaging, Nix, and build metadata. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Py as Python user
participant Pkg as flashvm (Python)
participant N as flashvm_native (PyO3)
participant T as types (ImageHandle/RunOptions)
participant V as vmm::run_vm
participant M as vmm::memory
participant K as vmm::kvm_ctx
participant B as vmm::boot
participant D as vmm::devices
participant L as vmm::event_loop
rect rgb(245,245,255)
Py->>Pkg: import flashvm
Py->>Pkg: prepare_image(meta: dict)
Pkg->>N: prepare_image(meta)
N->>T: ImageHandle::from_pydict(meta)
T-->>N: ImageHandle
N-->>Pkg: ImageHandle
Pkg-->>Py: ImageHandle
end
rect rgb(245,255,245)
Py->>Pkg: run(image, code, opts?)
Pkg->>N: run(image, code, opts)
N->>T: RunOptions::from_py(opts)
T-->>N: RunOptions
N->>V: run_vm(image, code, opts)
V->>M: GuestMem::create(mem_mb)
M-->>V: GuestMem
V->>K: KvmContext::new(cpus)
K-->>V: KvmContext
V->>B: load_kernel(gm, image.kernel, cmdline)
B-->>V: BootInfo
V->>D: attach_devices(cfg)
D-->>V: ok
V->>L: VmLoop::new()
L-->>V: VmLoop
V->>L: run_until_exit(timeout)
L-->>V: exit_status
V-->>N: RunResult
N-->>Pkg: RunResult
Pkg-->>Py: RunResult
end
note over V,L: TODOs: workspace image handling, device wiring, cmdline placement
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (15)
.gitignore (1)
1-1: Expand ignores for Python/PyO3 build artifacts.Avoid committing wheels, caches, and local env files from maturin/PyO3 work.
/target +.maturin/ +dist/ +build/ +__pycache__/ +*.py[cod] +*.egg-info/ +.venv/ +.envsrc/vmm/event_loop.rs (1)
8-11: Timeout parameter is ignored; define minimal semantics now.Even a stub honoring timeout (returning a distinct exit code on timeout) prevents API surprises later.
- pub fn run_until_exit(&mut self, _timeout_ms: Option<u64>) -> Result<i32> { - // TODO: dirigir o loop com event-manager - Ok(0) - } + pub fn run_until_exit(&mut self, timeout_ms: Option<u64>) -> Result<i32> { + // TODO: integrar event-manager; por ora, respeitar timeout se fornecido + if let Some(ms) = timeout_ms { + std::thread::sleep(std::time::Duration::from_millis(ms)); + // 124 segue o timeout(1) do coreutils; ajuste conforme a API desejada. + return Ok(124); + } + Ok(0) + }If you plan to use rust-vmm event-manager, I can sketch the loop wiring.
src/vmm/memory.rs (2)
9-13: Validate mem_mb and align to page size before creating the region.Prevents zero‑sized guests and non‑page‑aligned sizes.
pub fn create(mem_mb: u32) -> Result<Self> { - let size_u64 = (mem_mb as u64) * 1024 * 1024; + if mem_mb == 0 { anyhow::bail!("mem_mb must be >= 1"); } + let mut size_u64 = (mem_mb as u64) * 1024 * 1024; + const PAGE: u64 = 4096; + if size_u64 % PAGE != 0 { + size_u64 = ((size_u64 + PAGE - 1) / PAGE) * PAGE; + } let size: usize = size_u64.try_into().map_err(|_| anyhow::anyhow!("mem size too big"))?; let gm = GuestMemoryMmap::from_ranges(&[(GuestAddress(0), size)])?; Ok(Self { mem: gm }) }
5-5: Avoid leaking internals; provide accessors/traits instead of pub field.Keeps the memory wrapper flexible.
-pub struct GuestMem { pub mem: GuestMemoryMmap } +pub struct GuestMem { mem: GuestMemoryMmap } + +impl GuestMem { + pub fn mem(&self) -> &GuestMemoryMmap { &self.mem } +} + +impl AsRef<GuestMemoryMmap> for GuestMem { + fn as_ref(&self) -> &GuestMemoryMmap { &self.mem } +}pyproject.toml (2)
37-39: Replace placeholder URLs with the actual repository links.Improves package metadata on PyPI.
-[project.urls] -Homepage = "https://example.com/flashvm" -Source = "https://example.com/flashvm/repo" +[project.urls] +Homepage = "https://github.com/fullzer4/flashvm-py" +Source = "https://github.com/fullzer4/flashvm-py"
28-29: manylinux_2_28 limits glibc compatibility; confirm this target.If you want broader installability (e.g., Debian 9/RHEL7+), consider manylinux_2_17. Otherwise leave as‑is for newer distros.
src/vmm/platform_x86.rs (1)
1-4: Return ACPI blob via Result and gate on x86_64Change build_acpi_tables to return anyhow::Result<Vec> and add #[cfg(feature = "x86_64")] on the function. Confirmed: src/vmm/mod.rs already has #[cfg(feature = "x86_64")] pub mod platform_x86;.
-#[allow(dead_code)] -pub fn build_acpi_tables() { - // TODO: acpi_tables: DSDT, MADT, FADT mínimos -} +#[cfg(feature = "x86_64")] +#[allow(dead_code)] +pub fn build_acpi_tables() -> anyhow::Result<Vec<u8>> { + // TODO: acpi_tables: DSDT, MADT, FADT mínimos + Ok(Vec::new()) +}flake.nix (1)
23-23: Shell hook message is non-English; align with repo language or add context.“>> flashVM dev shell ativa” may confuse contributors. Consider English or a clearer message.
src/types.rs (2)
63-68: Key name mismatch: code reads "output", struct field is output_mode.If callers follow the field name, they’ll be ignored and silently default to "diff". The diff above prefers "output_mode" and keeps "output" as a compatibility fallback.
Please confirm the docs/examples use output_mode, not output.
75-81: RunResult: consider exit_status as i32 vs. u32 portability.i32 is fine; if you plan to surface raw wait statuses on Unix (including signal info), consider documenting semantics or mapping to 0..255 for cross‑platform clarity.
python/flashvm/__init__.py (1)
1-4: Import path/type-checking: remove type: ignore by adding stubs or re-exporting types.Prefer providing a .pyi for flashvm_native or re-exporting the PyO3 classes for better typing. Also confirm the extension lands under flashvm/flashvm_native.* for the relative import.
If you want, I’ll draft a minimal stub so mypy/pyright are happy.
src/vmm/mod.rs (1)
1-8: Feature gating vs target arch: ensure CI sets features; consider target_arch cfg if features aren’t passed.If callers don’t pass --features x86_64/aarch64, platform modules won’t compile in. You may want cfg(target_arch) for the platform modules and keep rust-vmm extras (acpi_tables/vm-fdt) feature-gated.
Confirm your build (maturin/nix) enables the right feature per target; I can sketch a CI matrix snippet if helpful.
src/vmm/devices.rs (1)
7-11: Add derives and a Default for partial construction.A
Defaultwithconsole_stdio: trueeases call sites; also derive Debug/Clone.-pub struct DevicesCfg { +#[derive(Debug, Clone)] +pub struct DevicesCfg { pub console_stdio: bool, pub rootfs: BlockSpec, pub workspace: Option<BlockSpec>, } + +impl Default for DevicesCfg { + fn default() -> Self { + Self { + console_stdio: true, + rootfs: BlockSpec { path: Default::default(), read_only: true }, + workspace: None, + } + } +}src/vmm/boot.rs (1)
2-2: Remove unused import.
KernelLoaderisn’t used.src/lib.rs (1)
21-27: GIL handling + typed args look good; watch options parsing fallback.
py.allow_threadsis correct, and accepting&ImageHandleis supported by PyO3. However,RunOptions::from_pycurrently swallows extract errors viaunwrap_or, turning bad user inputs into silent defaults. Prefer propagating type errors. PyO3 allows&T/PyRef<T>as function args. (pyo3.rs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockdocs/package-lock.jsonis excluded by!**/package-lock.jsonflake.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (64)
.gitattributes(0 hunks).github/workflows/deploy-docs.yml(0 hunks).gitignore(1 hunks)Cargo.toml(1 hunks)README.md(1 hunks)docker/Dockerfile.python-basic(0 hunks)docs/astro.config.mjs(0 hunks)docs/package.json(0 hunks)docs/src/content/config.ts(0 hunks)docs/src/content/docs/api.mdx(0 hunks)docs/src/content/docs/changelog.mdx(0 hunks)docs/src/content/docs/contributing.mdx(0 hunks)docs/src/content/docs/examples.mdx(0 hunks)docs/src/content/docs/guides/isolation.mdx(0 hunks)docs/src/content/docs/index.mdx(0 hunks)docs/src/content/docs/install.mdx(0 hunks)docs/src/content/docs/internals.mdx(0 hunks)docs/src/content/docs/license.mdx(0 hunks)docs/src/content/docs/quickstart.mdx(0 hunks)docs/src/content/docs/reference/host-tools.mdx(0 hunks)docs/src/content/docs/reference/result.mdx(0 hunks)docs/src/content/docs/reference/transports.mdx(0 hunks)docs/src/content/docs/troubleshooting.mdx(0 hunks)docs/src/content/docs/usage/artifacts.mdx(0 hunks)docs/src/content/docs/usage/image.mdx(0 hunks)docs/src/env.d.ts(0 hunks)docs/tsconfig.json(0 hunks)examples/basic_example.py(0 hunks)flake.nix(1 hunks)flashvm/__init__.py(0 hunks)flashvm/data/oci/blobs/sha256/39dbf954503055210a0ccbee25fdb599d9d3988b45da3978d7025c623db11909(0 hunks)flashvm/data/oci/blobs/sha256/89b2a69cdaa732c456834d53d9dc3e25bbeba1744fc3a79ba52493c28ed21d56(0 hunks)flashvm/data/oci/blobs/sha256/ca9e0bf09fc325818b461d3aa6975cf84ae2b4b9258d6cda7a2a7d39971f30f5(0 hunks)flashvm/data/oci/blobs/sha256/e4ba4d6f4188ade48f670414d944a9cd4454508c0adfdac240e9e81092fa831c(0 hunks)flashvm/data/oci/blobs/sha256/f34d2f7d34ea07609ad343af1c7dbebad6d22895fb3e68d4834fc18286de2d2a(0 hunks)flashvm/data/oci/blobs/sha256/f716fb09bc521d3afbb665994a43a221bf31b672d81fa2140e8b80a1c6c9b227(0 hunks)flashvm/data/oci/blobs/sha256/fbc8c3c9054a9d4f998cb1fecbfd6437a517df8b0c26c1682054a102182a6dc5(0 hunks)flashvm/data/oci/index.json(0 hunks)flashvm/data/oci/oci-layout(0 hunks)pyproject.toml(1 hunks)python/flashvm/__init__.py(1 hunks)src/config.rs(0 hunks)src/error.rs(1 hunks)src/image_resolver.rs(0 hunks)src/lib.rs(1 hunks)src/types.rs(1 hunks)src/vm_runner.rs(0 hunks)src/vmm/boot.rs(1 hunks)src/vmm/devices.rs(1 hunks)src/vmm/event_loop.rs(1 hunks)src/vmm/kvm_ctx.rs(1 hunks)src/vmm/memory.rs(1 hunks)src/vmm/mod.rs(1 hunks)src/vmm/platform_aarch64.rs(1 hunks)src/vmm/platform_x86.rs(1 hunks)src/vmm/run.rs(1 hunks)src/wheel_resources.rs(0 hunks)tests/__init__.py(0 hunks)tests/benchmark/test_performance.py(0 hunks)tests/conftest.py(0 hunks)tests/integration/test_complex_scenarios.py(0 hunks)tests/unit/test_basic_api.py(0 hunks)tests/unit/test_edge_cases.py(0 hunks)tests/unit/test_wheel_resources.py(0 hunks)
💤 Files with no reviewable changes (46)
- docs/src/content/docs/reference/result.mdx
- docs/src/content/docs/quickstart.mdx
- flashvm/data/oci/oci-layout
- docs/src/content/docs/usage/image.mdx
- docs/src/content/docs/api.mdx
- .github/workflows/deploy-docs.yml
- .gitattributes
- docs/tsconfig.json
- docs/src/content/docs/usage/artifacts.mdx
- docs/src/content/docs/guides/isolation.mdx
- flashvm/data/oci/blobs/sha256/e4ba4d6f4188ade48f670414d944a9cd4454508c0adfdac240e9e81092fa831c
- examples/basic_example.py
- flashvm/data/oci/blobs/sha256/f34d2f7d34ea07609ad343af1c7dbebad6d22895fb3e68d4834fc18286de2d2a
- tests/init.py
- flashvm/data/oci/index.json
- tests/unit/test_wheel_resources.py
- docs/src/content/docs/troubleshooting.mdx
- docs/src/content/config.ts
- flashvm/data/oci/blobs/sha256/f716fb09bc521d3afbb665994a43a221bf31b672d81fa2140e8b80a1c6c9b227
- docs/src/content/docs/changelog.mdx
- flashvm/data/oci/blobs/sha256/fbc8c3c9054a9d4f998cb1fecbfd6437a517df8b0c26c1682054a102182a6dc5
- docs/src/content/docs/license.mdx
- docs/src/content/docs/reference/host-tools.mdx
- docs/src/content/docs/install.mdx
- flashvm/init.py
- docs/astro.config.mjs
- docs/src/content/docs/index.mdx
- docs/src/env.d.ts
- docs/src/content/docs/internals.mdx
- docs/src/content/docs/contributing.mdx
- docs/src/content/docs/reference/transports.mdx
- flashvm/data/oci/blobs/sha256/ca9e0bf09fc325818b461d3aa6975cf84ae2b4b9258d6cda7a2a7d39971f30f5
- flashvm/data/oci/blobs/sha256/39dbf954503055210a0ccbee25fdb599d9d3988b45da3978d7025c623db11909
- src/wheel_resources.rs
- tests/unit/test_edge_cases.py
- src/image_resolver.rs
- tests/unit/test_basic_api.py
- docs/package.json
- docs/src/content/docs/examples.mdx
- tests/conftest.py
- flashvm/data/oci/blobs/sha256/89b2a69cdaa732c456834d53d9dc3e25bbeba1744fc3a79ba52493c28ed21d56
- tests/benchmark/test_performance.py
- src/config.rs
- docker/Dockerfile.python-basic
- src/vm_runner.rs
- tests/integration/test_complex_scenarios.py
🧰 Additional context used
🧬 Code graph analysis (5)
src/vmm/run.rs (4)
src/vmm/memory.rs (1)
create(9-14)src/vmm/event_loop.rs (1)
new(8-8)src/vmm/kvm_ctx.rs (1)
new(13-20)src/vmm/boot.rs (1)
load_kernel(11-25)
src/vmm/event_loop.rs (1)
src/vmm/kvm_ctx.rs (1)
new(13-20)
python/flashvm/__init__.py (1)
src/lib.rs (3)
flashvm_native(32-39)prepare_image(14-17)run(22-28)
src/vmm/mod.rs (1)
src/lib.rs (1)
run(22-28)
src/lib.rs (3)
src/types.rs (2)
from_pydict(17-31)from_py(46-70)src/vmm/run.rs (1)
run_vm(7-36)src/error.rs (1)
pyerr(5-5)
🔇 Additional comments (8)
Cargo.toml (3)
3-7: License change and version rollback need confirmation.You changed license to Apache-2.0 and version to 0.1.0. Ensure compatibility with prior published artifacts and contributor license expectations.
10-11: cdylib name is fine; ensure Python packaging matches.Confirm maturin/pyproject config installs the module as flashvm/flashvm_native so that python/flashvm/init.py can import .flashvm_native.
I can provide a quick check script to verify import paths after wheel build.
27-42: Pin rust-vmm git deps to commit SHAs and gate per-arch features.
- Cargo.toml (lines 27–42): replace "*" and git deps without rev (acpi_tables, vm-fdt, virtio-device, virtio-queue) with git = "...", rev = "" to prevent API drift.
- Ensure CI/build matrix enables the correct arch feature per target (pass --features x86_64 or aarch64).
- I can provide a Cargo config snippet and CI matrix env to enforce per-target features.
README.md (2)
51-60: Docs vs code: output modes are not implemented yet.The README advertises
output: "diff|all|none|paths"and returns stdout/stderr/artifacts, butrun_vmcurrently returns empty strings and no outputs_dir. Please gate the docs behind “coming soon” or implement minimal support.
72-79: Good alignment with rust‑vmm stack.The crate list matches the approach you’re taking (kvm-ioctls, vm-memory, linux-loader, vm-superio, vm-virtio, event-manager).
src/lib.rs (2)
10-10: Module surface tidy.Re-exports of
ImageHandle,RunOptions,RunResultfromtypesat the module root read clean and match README.
32-37: Bump pyo3 to >=0.23 before replacing macros.Repo currently pins pyo3 = 0.22 (Cargo.lock: 0.22.6); wrap_pyfunction_bound! was deprecated/renamed to wrap_pyfunction! in pyo3 0.23.0. (docs.rs)
- m.add_function(wrap_pyfunction_bound!(prepare_image, &m)?)?; - m.add_function(wrap_pyfunction_bound!(run, &m)?)?; + m.add_function(wrap_pyfunction!(prepare_image, m)?)?; + m.add_function(wrap_pyfunction!(run, m)?)?;src/vmm/run.rs (1)
30-31: Confirmed — VmLoop::run_until_exit exists and returns Result.
Defined in src/vmm/event_loop.rs as pub fn run_until_exit(&mut self, _timeout_ms: Option) -> Result; the call in src/vmm/run.rs yields an i32 after the?.
| anyhow = "1" | ||
| thiserror = "1" | ||
| serde = { version = "1", features = ["derive"] } | ||
| serde_json = "1" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin dependency versions; avoid "*" for stability and security.
Wildcards cause non-reproducible builds and surprise breakages. Pin semver ranges and vendored git crates by commit.
- Replace "*" with caret or tilde ranges (e.g., "0.x.y").
- Add Cargo.lock to VCS for this cdylib.
- For git deps (acpi_tables, vm-fdt, vm-virtio crates), pin rev = "" and consider a local workspace mirror.
🤖 Prompt for AI Agents
In Cargo.toml around lines 21-25, the dependency entries use broad version specs
(e.g., "1") which can lead to non-reproducible builds; update each to a pinned
semver range (use caret or tilde like ^1.2.3 or ~1.2.3) instead of bare or
wildcard versions, ensure you add Cargo.lock to the repository for this cdylib,
and for any git-sourced crates (acpi_tables, vm-fdt, vm-virtio) add an explicit
rev = "<commit-sha>" to lock to a commit (or replace with a published crate
version), optionally set up a local workspace mirror or vendor those crates to
avoid floating upstream changes.
flake.nix
Outdated
| systems = [ "x86_64-linux" "aarch64-linux" ]; | ||
| forAll = nixpkgs.lib.genAttrs systems; | ||
| in { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
OK on multi-arch; consider adding rust-overlay for reproducible toolchains.
Pin Rust toolchain via rust-overlay (rustc, cargo, clippy, rustfmt) to avoid drift across systems. Right now versions will float with nixpkgs.
Apply (illustrative) change by importing rust-overlay and using pkgs.rust-bin.stable.latest where appropriate.
flake.nix
Outdated
| pkgs.python312 pkgs.maturin pkgs.uv | ||
| pkgs.rustc pkgs.cargo pkgs.pkg-config | ||
| pkgs.docker pkgs.docker-buildx pkgs.gnutar pkgs.coreutils pkgs.jq | ||
| pkgs.umoci pkgs.skopeo | ||
| pkgs.erofs-utils pkgs.squashfsTools pkgs.zstd pkgs.lz4 | ||
| pkgs.busybox pkgs.cpio pkgs.e2fsprogs pkgs.cloud-hypervisor | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Dev shell is missing clippy/rustfmt and (potentially) libclang for bindgen-heavy crates.
- Add pkgs.clippy and pkgs.rustfmt.
- Some rust-vmm stacks/dep variants end up using bindgen; add llvmPackages.libclang and set BINDGEN_EXTRA_CLANG_ARGS only if needed.
I can draft a shellHook snippet to export BINDGEN_EXTRA_CLANG_ARGS if you confirm bindgen usage.
🤖 Prompt for AI Agents
In flake.nix around lines 16 to 22, the devShell inputs omit Rust developer
tools and libclang required for bindgen-heavy crates; update the list to include
pkgs.clippy and pkgs.rustfmt, also add llvmPackages.libclang to the inputs, and
if your crate set requires bindgen export BINDGEN_EXTRA_CLANG_ARGS from a
shellHook (only set the env var when bindgen is used) so bindgen can find the
correct clang include paths.
| } | ||
|
|
||
| pub type PyVMError = PyRuntimeError; | ||
| pub fn pyerr<E: std::fmt::Display>(e: E) -> PyErr { PyRuntimeError::new_err(e.to_string()) } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Retain structured errors internally; map to PyErr at the boundary.
Single PyRuntimeError is fine outwardly, but type‑safe errors inside ease diagnostics and testing.
-pub fn pyerr<E: std::fmt::Display>(e: E) -> PyErr { PyRuntimeError::new_err(e.to_string()) }
+pub fn pyerr<E: std::fmt::Display>(e: E) -> PyErr { PyRuntimeError::new_err(e.to_string()) }
+
+// Optional: keep an internal error enum (thiserror) and convert only at FFI boundaries.
+// #[derive(thiserror::Error, Debug)]
+// pub enum VMError { #[error("config: {0}")] Config(String), #[error(transparent)] Io(#[from] std::io::Error), /* ... */ }
+// pub fn to_py(err: impl std::fmt::Display) -> PyErr { PyRuntimeError::new_err(err.to_string()) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn pyerr<E: std::fmt::Display>(e: E) -> PyErr { PyRuntimeError::new_err(e.to_string()) } | |
| pub fn pyerr<E: std::fmt::Display>(e: E) -> PyErr { PyRuntimeError::new_err(e.to_string()) } | |
| // Optional: keep an internal error enum (thiserror) and convert only at FFI boundaries. | |
| // #[derive(thiserror::Error, Debug)] | |
| // pub enum VMError { #[error("config: {0}")] Config(String), #[error(transparent)] Io(#[from] std::io::Error), /* ... */ } | |
| // pub fn to_py(err: impl std::fmt::Display) -> PyErr { PyRuntimeError::new_err(err.to_string()) } |
🤖 Prompt for AI Agents
In src/error.rs around line 5, the pyerr function currently collapses any error
into a PyRuntimeError by accepting a generic Display, which loses structured
error types; change the approach so internal code uses a concrete, type-safe
Error enum (or Box<dyn std::error::Error + Send + Sync + 'static>) and stop
converting to PyErr everywhere — instead implement From<YourErrorEnum> (or
Into<PyErr>) that maps specific variants to a PyErr at the FFI/Python boundary,
and replace callers that need Python errors with an explicit conversion call at
the boundary; this preserves structured errors internally and only maps to PyErr
when crossing into Python.
| pub fn from_pydict<'py>(d: &pyo3::Bound<'py, PyDict>) -> PyResult<Self> { | ||
| // TODO: validação robusta + normalização de paths | ||
| Ok(Self { | ||
| kernel: d | ||
| .get_item("kernel")? | ||
| .ok_or_else(|| pyo3::exceptions::PyValueError::new_err("missing kernel"))? | ||
| .extract()?, | ||
| rootfs_img: d | ||
| .get_item("rootfs_img")? | ||
| .ok_or_else(|| pyo3::exceptions::PyValueError::new_err("missing rootfs_img"))? | ||
| .extract()?, | ||
| initrd: d.get_item("initrd")?.map(|v| v.extract()).transpose()?, | ||
| cache_key: d.get_item("cache_key")?.map(|v| v.extract()).transpose()?, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ImageHandle: accept pathlib.Path and normalize to absolute paths.
Current API only extracts strings and defers validation. Accept PathLike (pathlib.Path, str), canonicalize, and validate existence to fail fast.
If you want, I’ll provide a small helper that resolves PyAny to PathBuf and returns PyValueError on invalid inputs.
| #[allow(dead_code)] | ||
| pub fn build_fdt() { | ||
| // TODO: vm-fdt: memória, CPUs, virtio-mmio, chosen/bootargs | ||
| } No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Stabilize the API: return the FDT bytes now to avoid a later breaking change.
Mirror other modules using anyhow::Result and feature‑gate here.
-#[allow(dead_code)]
-pub fn build_fdt() {
- // TODO: vm-fdt: memória, CPUs, virtio-mmio, chosen/bootargs
-}
+#[cfg(feature = "aarch64")]
+#[allow(dead_code)]
+pub fn build_fdt() -> anyhow::Result<Vec<u8>> {
+ // TODO: vm-fdt: memória, CPUs, virtio-mmio, chosen/bootargs
+ Ok(Vec::new())
+}Please confirm src/vmm/mod.rs exports this under the aarch64 feature.
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== rg: find occurrences of build_fdt ==="
rg -n --hidden --no-ignore --color=never "build_fdt" || true
echo
echo "=== rg: find platform_aarch64 references ==="
rg -n --hidden --no-ignore --color=never "platform_aarch64" || true
echo
echo "=== show: src/vmm/platform_aarch64.rs (if present) ==="
if [ -f src/vmm/platform_aarch64.rs ]; then
sed -n '1,240p' src/vmm/platform_aarch64.rs
else
echo "MISSING: src/vmm/platform_aarch64.rs"
fi
echo
echo "=== show: src/vmm/mod.rs (if present) ==="
if [ -f src/vmm/mod.rs ]; then
sed -n '1,400p' src/vmm/mod.rs
else
echo "MISSING: src/vmm/mod.rs"
fi
echo
echo "=== rg: look for cfg(feature = \"aarch64\") in src/vmm ==="
rg -n --hidden --no-ignore --color=never '#\[cfg\(feature\s*=\s*\"aarch64\"\)\]' src/vmm || true
echo
echo "=== rg: look for pub use / re-exports of build_fdt / platform_aarch64 ==="
rg -n --hidden --no-ignore --color=never 'pub\s+(use|mod)\s+.*platform_aarch64' || true
rg -n --hidden --no-ignore --color=never 'pub\s+fn\s+build_fdt' || true
echo
echo "=== rg: search Cargo.toml / features for aarch64 ==="
rg -n --hidden --no-ignore --color=never 'aarch64' Cargo.toml || trueLength of output: 1073
Stabilize the API: return the FDT bytes (anyhow::Result<Vec>) now to avoid a later breaking change.
Change src/vmm/platform_aarch64.rs to:
pub fn build_fdt() -> anyhow::Result<Vec> { /* TODO: build FDT */ Ok(Vec::new()) }
src/vmm/mod.rs already gates the module: #[cfg(feature = "aarch64")] pub mod platform_aarch64; — per-function #[cfg] is redundant.
🤖 Prompt for AI Agents
In src/vmm/platform_aarch64.rs around lines 1 to 4, change the function
signature to return anyhow::Result<Vec<u8>> (pub fn build_fdt() ->
anyhow::Result<Vec<u8>>) and return a placeholder Ok(Vec::new()) for now; add a
use anyhow::Result or ensure anyhow is in scope (or reference anyhow::Result
explicitly), and leave module-level cfg in src/vmm/mod.rs as-is so you don't add
per-function #[cfg]s; keep existing annotations like #[allow(dead_code)] if
desired.
| let gm = GuestMem::create(opts.mem_mb)?; | ||
| let _kvmx = KvmContext::new(opts.cpus)?; | ||
|
|
There was a problem hiding this comment.
Blocker: validate cpus/mem before creating VM (avoid zero‑vCPU/zero‑mem).
Currently opts.cpus or opts.mem_mb can be zero (Python inputs default, or user mistakes), which leads to an empty vCPU set and potentially invalid guest memory, causing boot failure. Add hard guards. Also, upstream KvmContext::new (see kvm_ctx.rs lines 12–19 in snippets) appears to call create_vcpu(0) for each vCPU, which would duplicate vCPU IDs; until that’s fixed, ensure cpus >= 1 here and follow up there.
Apply this diff in this function’s preamble:
use anyhow::Result;
+use anyhow::ensure;
@@
pub fn run_vm(image: &ImageHandle, code: &str, opts: &RunOptions) -> Result<RunResult> {
- // 1) memória + KVM
+ // 1) sanity checks
+ ensure!(opts.cpus > 0, "cpus must be >= 1");
+ ensure!(opts.mem_mb >= 64, "mem_mb must be >= 64 MiB");
+ // 2) memória + KVM
let gm = GuestMem::create(opts.mem_mb)?;
let _kvmx = KvmContext::new(opts.cpus)?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let gm = GuestMem::create(opts.mem_mb)?; | |
| let _kvmx = KvmContext::new(opts.cpus)?; | |
| use anyhow::ensure; | |
| use anyhow::Result; | |
| pub fn run_vm(image: &ImageHandle, code: &str, opts: &RunOptions) -> Result<RunResult> { | |
| // 1) sanity checks | |
| ensure!(opts.cpus > 0, "cpus must be >= 1"); | |
| ensure!(opts.mem_mb >= 64, "mem_mb must be >= 64 MiB"); | |
| // 2) memória + KVM | |
| let gm = GuestMem::create(opts.mem_mb)?; | |
| let _kvmx = KvmContext::new(opts.cpus)?; |
🤖 Prompt for AI Agents
In src/vmm/run.rs around lines 9–11, add hard validation checks before creating
GuestMem and KvmContext: verify opts.cpus is >= 1 and opts.mem_mb is >= 1, and
return an early Err with a clear message if either is zero (do not call
GuestMem::create or KvmContext::new when invalid). Place these guards at the
function preamble immediately before the existing GuestMem and KvmContext
construction so you never construct a VM with zero vCPUs or zero memory; ensure
the error paths use the same error type returned by the function. Also add a
brief comment referencing the upstream KvmContext issue (create_vcpu(0)
duplication) so maintainers know why the cpus >= 1 check is required here.
| let kernel_path = PathBuf::from(&image.kernel); | ||
| let _boot = boot::load_kernel(&gm.mem, &kernel_path, &cmdline)?; | ||
|
|
There was a problem hiding this comment.
Kernel load result is unused; boot path not wired (regs/boot params/entry).
You load the kernel but do not configure vCPU regs, zero page / PVH start info, or write the cmdline into guest memory. linux-loader expects the VMM to write cmdline and boot params; consider using load_cmdline and a boot configurator, and plumb the entry point for vCPU 0. Otherwise the guest will not boot. (docs.rs)
🤖 Prompt for AI Agents
In src/vmm/run.rs around lines 21 to 23, you call boot::load_kernel but ignore
its result and never wire kernel boot state into the VMM; fix by capturing the
loader result and using it to configure the vCPU registers and guest memory:
call the loader's helpers (e.g., load_cmdline / boot configurator from
linux-loader) to write the kernel cmdline into guest memory, populate the zero
page or PVH start-info/boot params expected by the chosen boot protocol, and set
the vCPU entry point (RIP) and any required segment/register state for vCPU 0
from the returned entry/boot info; ensure you use gm.mem when writing
cmdline/boot structures and propagate any errors up instead of discarding the
loader result.
| // 4) anexar devices (serial + vda=rootfs RO + vdb=workspace RW) | ||
| // TODO: criar workspace.img e gravar código/entrypoint | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wire basic devices now (rootfs + console) behind a tiny config.
You’ve got the TODO; at minimum, invoke the stub so the path compiles end‑to‑end and is easy to extend.
Apply this diff:
-use super::{kvm_ctx::KvmContext, memory::GuestMem, boot, event_loop::VmLoop};
+use super::{kvm_ctx::KvmContext, memory::GuestMem, boot, event_loop::VmLoop};
+use super::devices::{DevicesCfg, BlockSpec, attach_devices};
@@
- // 4) anexar devices (serial + vda=rootfs RO + vdb=workspace RW)
- // TODO: criar workspace.img e gravar código/entrypoint
+ // 4) anexar devices (serial + vda=rootfs RO + vdb=workspace RW)
+ let dev_cfg = DevicesCfg {
+ console_stdio: true,
+ rootfs: BlockSpec { path: image.rootfs_img.clone(), read_only: true },
+ workspace: None, // TODO: criar workspace.img e gravar código/entrypoint
+ };
+ attach_devices(&dev_cfg)?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 4) anexar devices (serial + vda=rootfs RO + vdb=workspace RW) | |
| // TODO: criar workspace.img e gravar código/entrypoint | |
| use super::{kvm_ctx::KvmContext, memory::GuestMem, boot, event_loop::VmLoop}; | |
| use super::devices::{DevicesCfg, BlockSpec, attach_devices}; | |
| // 4) anexar devices (serial + vda=rootfs RO + vdb=workspace RW) | |
| let dev_cfg = DevicesCfg { | |
| console_stdio: true, | |
| rootfs: BlockSpec { path: image.rootfs_img.clone(), read_only: true }, | |
| workspace: None, // TODO: criar workspace.img e gravar código/entrypoint | |
| }; | |
| attach_devices(&dev_cfg)?; |
|
|
||
|
|
||
| // 6) coletar stdout/stderr + artefatos do workspace | ||
| Ok(RunResult { stdout: String::new(), stderr: String::new(), exit_status: status, outputs_dir: None }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return real stdout/stderr by plumbing the serial backend.
Right now both are empty. Ensure the serial device in devices writes TX to an in‑memory buffer you can retrieve here; otherwise the Python API promises in README won’t match behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
src/types.rs (2)
9-15: Accept PathLike and normalize paths in ImageHandle.Support pathlib.Path/PathLike and convert to strings; fail fast on bad types. This matches Python ergonomics and avoids silent mis-specification.
Apply this diff to use a helper for path extraction:
- let kernel = d.get_item("kernel")?.map(|v| v.extract()).transpose()?; - let rootfs_img = d.get_item("rootfs_img")?.map(|v| v.extract()).transpose()?; + let kernel = d.get_item("kernel")?.map(|v| extract_pathlike_to_string(&v)).transpose()?; + let rootfs_img = d.get_item("rootfs_img")?.map(|v| extract_pathlike_to_string(&v)).transpose()?; @@ - initrd: d.get_item("initrd")?.map(|v| v.extract()).transpose()?, + initrd: d.get_item("initrd")?.map(|v| extract_pathlike_to_string(&v)).transpose()?,Add this helper (outside the shown range):
use std::path::PathBuf; fn extract_pathlike_to_string(v: &pyo3::Bound<pyo3::PyAny>) -> PyResult<String> { let pb: PathBuf = v.extract()?; Ok(pb.to_string_lossy().into_owned()) }Also applies to: 27-28, 34-34
66-91: Make RunOptions parsing strict; reject cpus=0/mem_mb=0; validate output_mode.unwrap_or() swallows type/range errors; invalid values pass through and fail later.
- pub fn from_py<'py>(d: Option<&pyo3::Bound<'py, PyDict>>) -> PyResult<Self> { - let cpus: u8 = match d { - Some(x) => x.get_item("cpus")?.map(|v| v.extract().unwrap_or(1)).unwrap_or(1), - None => 1, - }; - let mem_mb: u32 = match d { - Some(x) => x.get_item("mem_mb")?.map(|v| v.extract().unwrap_or(512)).unwrap_or(512), - None => 512, - }; - let timeout_ms: Option<u64> = match d { - Some(x) => x - .get_item("timeout_ms")? - .map(|v| v.extract().unwrap_or(0)) - .filter(|&t| t > 0), - None => None, - }; - let output_mode: String = match d { - Some(x) => x - .get_item("output")? - .map(|v| v.extract().unwrap_or("diff".to_string())) - .unwrap_or("diff".into()), - None => "diff".into(), - }; - Ok(Self { cpus, mem_mb, timeout_ms, output_mode }) - } + pub fn from_py<'py>(d: Option<&pyo3::Bound<'py, PyDict>>) -> PyResult<Self> { + let mut cpus: u8 = 1; + let mut mem_mb: u32 = 512; + let mut timeout_ms: Option<u64> = None; + let mut output_mode: String = "diff".into(); + + if let Some(x) = d { + if let Some(v) = x.get_item("cpus")? { + let parsed: u8 = v.extract()?; + if parsed == 0 { + return Err(pyo3::exceptions::PyValueError::new_err("cpus must be >= 1")); + } + cpus = parsed; + } + if let Some(v) = x.get_item("mem_mb")? { + let parsed: u32 = v.extract()?; + if parsed == 0 { + return Err(pyo3::exceptions::PyValueError::new_err("mem_mb must be > 0")); + } + mem_mb = parsed; + } + if let Some(v) = x.get_item("timeout_ms")? { + let t: u64 = v.extract()?; + if t > 0 { + timeout_ms = Some(t); + } + } + // Prefer "output_mode"; fall back to legacy "output". + if let Some(v) = x.get_item("output_mode")?.or(x.get_item("output")?) { + output_mode = v.extract()?; + } + } + + match output_mode.as_str() { + "diff" | "all" | "none" | "paths" => {} + other => { + return Err(pyo3::exceptions::PyValueError::new_err(format!( + "invalid output_mode: {other} (expected one of: diff|all|none|paths)" + ))); + } + } + + Ok(Self { cpus, mem_mb, timeout_ms, output_mode }) + }src/vmm/boot.rs (3)
8-8: Do not treatkernel_loadas the vCPU entry point; rename to avoid misuse.
KernelLoaderResult.kernel_loadis not the jump target for all protocols. Using it asentryis misleading and risks incorrect CPU state setup. Rename the field and defer computing the real entry until after protocol-specific setup.- pub struct BootInfo { pub entry: GuestAddress, pub cmdline_addr: GuestAddress } + pub struct BootInfo { pub kernel_load: GuestAddress, pub cmdline_addr: GuestAddress } @@ - Ok(BootInfo { entry: k.kernel_load, cmdline_addr }) + Ok(BootInfo { kernel_load: k.kernel_load, cmdline_addr })Also applies to: 30-30
11-18: Implement protocol-aware boot (bzImage/PVH/ELF) and populate boot params + vCPU regs.Loading an ELF and jumping to
kernel_loadwill not boot a stock x86_64 Linux. Detect or choose the protocol (bzImage preferred on x86_64), write boot params (including cmdline pointer), and configure registers per protocol before entering the kernel.
- BzImage: load with the bzImage loader, write boot_params, set cmdline_ptr, set RIP/RSP/RFLAGS as required.
- PVH: build PVH start info, set entry accordingly.
- ELF: only valid if using PVH-capable ELF; otherwise reject with a clear error.
linux-loader KernelLoaderResult docs; rust-vmm bzImage loader example; rust-vmm load_cmdline usage
24-28: Use the loader helper to write the cmdline and avoid hardcoding the address.Prefer
linux_loader::loader::load_cmdlineand compute an aligned location that doesn’t collide with the kernel/initrd and respects protocol limits. Also, ensure the cmdline pointer is written into the protocol’s boot state (e.g., boot_params for bzImage).- let cstr = cmd.as_cstring()?; - // TODO(boot): calcular cmdline_addr conforme layout real (abaixo de 1MiB, alinhamento conforme protocolo) - let cmdline_addr = GuestAddress(0x20000); - gm.write_slice(cstr.to_bytes_with_nul(), cmdline_addr)?; + use linux_loader::loader::load_cmdline; + let cstr = cmd.as_cstring()?; + // TODO(boot): pick protocol-safe, aligned addr that doesn't overlap kernel/initrd + let cmdline_addr = GuestAddress(0x20000); + load_cmdline::<GuestMemoryMmap>(gm, cmdline_addr, &cstr)?;Tip (non-blocking): place cmdline near the top of the first RAM region, aligned (e.g., 16B), below 4 GiB, and below 1 MiB for legacy if following bzImage guidance.
flake.nix (2)
17-29: DevShell: add clippy/rustfmt (+ libclang if bindgen appears).Improves DX; avoids drift with lint/format. libclang only if bindgen is used.
default = pkgs.mkShell { buildInputs = [ - pkgs.rustc - pkgs.cargo + pkgs.rustc + pkgs.cargo + pkgs.clippy + pkgs.rustfmt pkgs.maturin pkgs.python311 pkgs.pkg-config pkgs.openssl + # Uncomment if any dependency uses bindgen: + # pkgs.llvmPackages.libclang ]; shellHook = '' export PYO3_PYTHON=${pkgs.python311}/bin/python3 + # If using bindgen, uncomment: + # export BINDGEN_EXTRA_CLANG_ARGS="--sysroot=${pkgs.llvmPackages.libclang.lib}/lib/clang/${pkgs.llvmPackages.libclang.version}/include" ''; };
4-12: Pin Rust toolchain via rust-overlay for reproducibility.Avoids implicit rustc/cargo drift across nixpkgs revisions.
inputs.nixpkgs.url = "github:NixOS/nixpkgs/nixos-24.05"; + inputs.rust-overlay.url = "github:oxalica/rust-overlay"; @@ - outputs = { self, nixpkgs }: + outputs = { self, nixpkgs, rust-overlay }: let systems = [ "x86_64-linux" "aarch64-linux" ]; forAllSystems = f: - nixpkgs.lib.genAttrs systems (system: - let pkgs = import nixpkgs { inherit system; }; in f pkgs system); + nixpkgs.lib.genAttrs systems (system: + let + pkgs = import nixpkgs { + inherit system; + overlays = [ (import rust-overlay) ]; + }; + in f pkgs system);Optionally replace rustc/cargo with a single pinned toolchain in the devShell:
- pkgs.rustc - pkgs.cargo + pkgs.rust-bin.stable.latest.default
🧹 Nitpick comments (9)
src/types.rs (1)
42-48: Unify error messages in English and clarify missing fields.Current messages are mixed-language and vague.
- Err(pyo3::exceptions::PyValueError::new_err( - "base='embedded' requer que o chamador Python resolva paths (kernel/rootfs_img) dos assets do pacote e reenvie no meta", - )) + Err(pyo3::exceptions::PyValueError::new_err( + "base='embedded' requires the Python caller to resolve package asset paths (kernel/rootfs_img) and provide them in meta", + )) @@ - Err(pyo3::exceptions::PyValueError::new_err( - "missing kernel/rootfs_img", - )) + Err(pyo3::exceptions::PyValueError::new_err( + "missing required fields: kernel and/or rootfs_img", + ))src/lib.rs (1)
17-21: warnings.warn: pass category via kwargs and set stacklevel.Improves Python-side UX (points at caller) and avoids positional-arg ambiguity.
- warnings.call_method1( - "warn", - ( - "DEV-only base. For production, prefer `flashvm build -p ...` to create a pinned OCI image.", - warn_type, - ), - )?; + let kwargs = pyo3::types::PyDict::new_bound(py); + kwargs.set_item("category", warn_type)?; + kwargs.set_item("stacklevel", 2)?; + warnings.call_method( + "warn", + ("DEV-only base. For production, prefer `flashvm build -p ...` to create a pinned OCI image.",), + Some(&kwargs), + )?;Also applies to: 21-29
src/vmm/boot.rs (4)
21-22: Increase cmdline buffer to typical Linux max (2048).1024 is likely too small; many kernels use CONFIG_CMDLINE_SIZE=2048+. Safer default: 2048 with graceful error if the input exceeds capacity.
- let mut cmd = Cmdline::new(1024)?; + let mut cmd = Cmdline::new(2048)?;
2-2: Remove unused import and prepare forload_cmdline.
KernelLoaderisn’t used; importload_cmdlineinstead if adopting the helper.-use linux_loader::loader::{Elf, KernelLoader}; +use linux_loader::loader::{Elf, load_cmdline};
11-11: Generalize the signature for flexibility.Accept
AsRef<Path>forkernel_path(ergonomics) and considerGM: GuestMemoryinstead ofGuestMemoryMmapto decouple from the concrete backend.-pub fn load_kernel(gm: &GuestMemoryMmap, kernel_path: &Path, cmdline: &str) -> Result<BootInfo> { +pub fn load_kernel<P: AsRef<Path>>(gm: &GuestMemoryMmap, kernel_path: P, cmdline: &str) -> Result<BootInfo> { @@ - let mut kernel_image = std::fs::File::open(kernel_path)?; + let mut kernel_image = std::fs::File::open(kernel_path.as_ref())?;(Optional follow-up)
pub fn load_kernel<GM: vm_memory::GuestMemory, P: AsRef<Path>>(gm: &GM, kernel_path: P, cmdline: &str) -> Result<BootInfo> { /* ... */ }
8-8: Derive common traits onBootInfo.Handy for logging and copying tiny structs.
+#[derive(Debug, Copy, Clone)] pub struct BootInfo { pub entry: GuestAddress, pub cmdline_addr: GuestAddress }package.nix (1)
8-11: Consider adding meta (description, license, maintainers).Helps downstream users and Nixpkgs-style hygiene.
py.buildPythonPackage { pname = "flashvm"; version = "0.1.0"; + + meta = with pkgs.lib; { + description = "flashvm Python bindings built with PyO3/maturin"; + license = licenses.mit; # adjust + maintainers = [ maintainers.yourGitHubHandle ]; # adjust + platforms = platforms.linux; + };default.nix (1)
1-2: Provide a clearer failure if default is missing.Small QoL: throw a helpful error instead of a cryptic missing-attr failure.
-(let flake = builtins.getFlake (toString ./.); in - flake.packages.${builtins.currentSystem}.default) +(let + flake = builtins.getFlake (toString ./.); + system = builtins.currentSystem; +in + (flake.packages.${system}.default + or (throw "flake.packages.${system}.default is missing")))flake.nix (1)
8-12: Optional: derive systems from lib to scale later.Keeps the list DRY if you expand platforms.
- systems = [ "x86_64-linux" "aarch64-linux" ]; + systems = with nixpkgs.lib.systems; [ "x86_64-linux" "aarch64-linux" ]; + # or: systems = nixpkgs.lib.systems.flakeExposed;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
default.nix(1 hunks)flake.nix(1 hunks)package.nix(1 hunks)src/lib.rs(1 hunks)src/types.rs(1 hunks)src/vmm/boot.rs(1 hunks)src/vmm/devices.rs(1 hunks)src/vmm/event_loop.rs(1 hunks)src/vmm/kvm_ctx.rs(1 hunks)src/vmm/memory.rs(1 hunks)src/vmm/run.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/vmm/event_loop.rs
- src/vmm/run.rs
- src/vmm/memory.rs
- src/vmm/devices.rs
- src/vmm/kvm_ctx.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/vmm/boot.rs (1)
src/vmm/kvm_ctx.rs (1)
new(13-29)
src/lib.rs (3)
src/types.rs (2)
from_pydict(20-52)from_py(67-91)src/vmm/run.rs (1)
run_vm(7-42)src/error.rs (1)
pyerr(5-5)
🔇 Additional comments (8)
src/types.rs (2)
56-64: RunOptions shape looks good.
95-102: RunResult looks fine as a thin DTO.src/lib.rs (4)
5-7: Module layout LGTM.
10-10: Public types import LGTM.
39-46: GIL release + error mapping LGTM.allow_threads around VM run and central pyerr mapping look correct.
50-56: Module registration LGTM.flake.nix (1)
13-16: Packages wiring LGTM.Delegating to package.nix per-system is clean.
package.nix (1)
12-20: Add cargoHash for hermetic buildsAdd cargoHash next to src so maturin/buildRustPackage uses vendored crates and does not fetch crates during Nix builds (required for reproducible/offline builds). (github.com)
File: package.nix
Lines: 12-20format = "pyproject"; src = ./.; + # Required for reproducible/offline Rust crate builds via maturin + cargoHash = "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="; # TODO: replace nativeBuildInputs = [Run this locally and paste the "got: sha256-..." value back into cargoHash:
#!/bin/bash set -euo pipefail sys="$(nix eval --raw --expr 'builtins.currentSystem')" # Expect a failed build that prints the correct cargoHash; copy the "got: sha256-..." value. nix build ".#packages.${sys}.default"
| doCheck = false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a cheap import check instead of fully disabling tests.
Keep doCheck = false if needed, but ensure wheels import.
doCheck = false;
+ pythonImportsCheck = [ "flashvm" ];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| doCheck = false; | |
| } | |
| doCheck = false; | |
| pythonImportsCheck = [ "flashvm" ]; | |
| } |
🤖 Prompt for AI Agents
In package.nix around lines 22-23, doCheck is currently disabled; instead of
fully skipping tests, add a minimal import check to ensure built wheels import
correctly by adding a small checkPhase (or postInstallCheck) that runs the
package's Python interpreter to import the package module(s) (e.g., python -c
"import <module>") for each wheel; keep doCheck = false if the full test-suite
must remain disabled but add this cheap import check so build failures from
broken wheels are caught early.
| @@ -0,0 +1,31 @@ | |||
| use anyhow::Result; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add context to I/O/loader errors for easier debugging.
Attach file path and operation details to errors.
-use anyhow::Result;
+use anyhow::{Result, Context};
@@
- let mut kernel_image = std::fs::File::open(kernel_path)?;
- let k = Elf::load(gm, None, &mut kernel_image, None)?;
+ let mut kernel_image = std::fs::File::open(kernel_path)
+ .with_context(|| format!("opening kernel image at {}", kernel_path.display()))?;
+ let k = Elf::load(gm, None, &mut kernel_image, None)
+ .context("loading kernel with ELF loader")?;Also applies to: 16-17
🤖 Prompt for AI Agents
In src/vmm/boot.rs around lines 1 and 16-17, the I/O and loader error returns
lack contextual information; update any fs/loader calls to attach file path and
operation details using anyhow::Context (e.g., .with_context(|| format!("reading
boot binary '{}'", path)) or .context("opening initrd for path ...")), and wrap
loader errors similarly so every Err carries which file/path and which operation
failed for easier debugging.
Summary by CodeRabbit
Breaking Changes
New Features
Documentation
Chores
Tests