Add some VMM_DESTROY_VM polish to bhyve-api

bin/propolis-utils/src/bin/cpuid-gen.rs

@@ -6,15 +6,15 @@ use clap::Parser;
 
 fn create_vm() -> anyhow::Result<VmmFd> {
     let name = format!("cpuid-gen-{}", std::process::id());
-    let mut req = bhyve_api::vm_create_req::new(&name);
+    let mut req =
+        bhyve_api::vm_create_req::new(name.as_bytes()).expect("valid VM name");
 
     let ctl = VmmCtlFd::open()?;
     let _ = unsafe { ctl.ioctl(bhyve_api::VMM_CREATE_VM, &mut req) }?;
 
     let vm = VmmFd::open(&name).map_err(|e| {
         // Attempt to manually destroy the VM if we cannot open it
-        let mut destroy = bhyve_api::vm_destroy_req::new(&name);
-        let _ = unsafe { ctl.ioctl(bhyve_api::VMM_DESTROY_VM, &mut destroy) };
+        let _ = ctl.vm_destroy(name.as_bytes());
         e
     })?;
 

crates/bhyve-api/src/lib.rs

@@ -103,6 +103,13 @@ impl VmmCtlFd {
         Ok(req)
     }
 
+    /// Destroy VM instance
+    pub fn vm_destroy(&self, name: &[u8]) -> Result<()> {
+        let mut req = vm_destroy_req::new(name)?;
+        unsafe { self.ioctl(ioctls::VMM_DESTROY_VM, &mut req)? };
+        Ok(())
+    }
+
     /// Check VMM ioctl command against those known to not require any
     /// copyin/copyout to function.
     const fn ioctl_usize_safe(cmd: i32) -> bool {

crates/bhyve-api/sys/src/structs.rs

@@ -1,3 +1,4 @@
+use std::io::{Error, ErrorKind, Result};
 use std::os::raw::{c_int, c_uint, c_void};
 
 use libc::size_t;
@@ -430,11 +431,20 @@ pub struct vm_legacy_cpuid {
 pub const VM_MAX_NAMELEN: usize = 128;
 pub const VM_MAX_SEG_NAMELEN: usize = 128;
 
-// Copy VM name, paying no heed to whether a trailing NUL is left in the
-// destination byte slice.  The kernel will do that error handling.
-fn copy_name(field: &mut [u8], name: &str) {
-    let copy_len = name.len().min(field.len());
-    field[..copy_len].copy_from_slice(name.as_bytes());
+/// Copy VM name into array appropriately sized for create/destroy request.
+/// Advanced checks are left to the kernel logic consuming that value.
+fn validate_name(value: &[u8]) -> Result<[u8; VM_MAX_NAMELEN]> {
+    let mut buf = [0u8; VM_MAX_NAMELEN];
+
+    if value.len() > buf.len() {
+        return Err(Error::new(
+            ErrorKind::InvalidInput,
+            "name length exceeds VM_MAX_NAMELEN",
+        ));
+    }
+
+    buf[..(value.len())].copy_from_slice(value);
+    Ok(buf)
 }
 
 #[repr(C)]
@@ -449,10 +459,8 @@ impl Default for vm_create_req {
     }
 }
 impl vm_create_req {
-    pub fn new(name: &str) -> Self {
-        let mut res = Self::default();
-        copy_name(&mut res.name, name);
-        res
+    pub fn new(name: &[u8]) -> Result<Self> {
+        Ok(Self { name: validate_name(name)?, flags: 0 })
     }
 }
 
@@ -476,10 +484,8 @@ impl Default for vm_destroy_req {
     }
 }
 impl vm_destroy_req {
-    pub fn new(name: &str) -> Self {
-        let mut res = Self::default();
-        copy_name(&mut res.name, name);
-        res
+    pub fn new(name: &[u8]) -> Result<Self> {
+        Ok(Self { name: validate_name(name)? })
     }
 }
 

lib/propolis/src/vmm/hdl.rs

@@ -44,7 +44,7 @@ pub struct CreateOpts {
 pub(crate) fn create_vm(name: &str, opts: CreateOpts) -> Result<VmmHdl> {
     let ctl = bhyve_api::VmmCtlFd::open()?;
 
-    let mut req = bhyve_api::vm_create_req::new(name);
+    let mut req = bhyve_api::vm_create_req::new(name.as_bytes())?;
     if opts.use_reservoir {
         req.flags |= bhyve_api::VCF_RESERVOIR_MEM;
     }
@@ -58,12 +58,10 @@ pub(crate) fn create_vm(name: &str, opts: CreateOpts) -> Result<VmmHdl> {
         }
 
         // try to nuke(!) the existing vm
-        let mut dreq = bhyve_api::vm_destroy_req::new(name);
-        let _ = unsafe { ctl.ioctl(bhyve_api::VMM_DESTROY_VM, &mut dreq) }
-            .or_else(|e| match e.kind() {
-                ErrorKind::NotFound => Ok(0),
-                _ => Err(e),
-            })?;
+        ctl.vm_destroy(name.as_bytes()).or_else(|e| match e.kind() {
+            ErrorKind::NotFound => Ok(()),
+            _ => Err(e),
+        })?;
 
         // now attempt to create in its presumed absence
         let _ = unsafe { ctl.ioctl(bhyve_api::VMM_CREATE_VM, &mut req) }?;
@@ -82,18 +80,6 @@ pub(crate) fn create_vm(name: &str, opts: CreateOpts) -> Result<VmmHdl> {
     })
 }
 
-/// Destroys the virtual machine matching the provided `name`.
-fn destroy_vm_impl(name: &str) -> Result<()> {
-    let ctl = bhyve_api::VmmCtlFd::open()?;
-    let mut dreq = bhyve_api::vm_destroy_req::new(name);
-    let _ = unsafe { ctl.ioctl(bhyve_api::VMM_DESTROY_VM, &mut dreq) }
-        .or_else(|e| match e.kind() {
-            ErrorKind::NotFound => Ok(0),
-            _ => Err(e),
-        })?;
-    Ok(())
-}
-
 /// A wrapper around a file which must uphold the guarantee that the underlying
 /// structure may not be truncated.
 pub struct VmmFile(File);
@@ -403,7 +389,11 @@ impl VmmHdl {
         // If that failed (which may occur on older platforms without
         // self-destruction), then fall back to performing the destroy through
         // the vmmctl device.
-        destroy_vm_impl(&self.name)
+        let ctl = bhyve_api::VmmCtlFd::open()?;
+        ctl.vm_destroy(self.name.as_bytes()).or_else(|e| match e.kind() {
+            ErrorKind::NotFound => Ok(()),
+            _ => Err(e),
+        })
     }
 
     /// Set whether instance should auto-destruct when closed