From 9975e79f78faac4946feb854bcaa9db5f5d76151 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Thu, 31 Mar 2022 15:13:16 -0700 Subject: [PATCH] devices: vsock: simplify device path to a PathBuf The vhost-vsock device had some special-case code to handle a file descriptor as the vhost device rather than a path. However, crosvm already has base::open_file(), which has special handling for magic /proc/self/fd/ paths and treats them as already-open file descriptors without requiring a separate command-line option. This change converts the vhost vsock device to accept only a path instead of a special path/fd enum, using base::open_file() to support passing already-open file descriptors via the /proc/self/fd path format. The existing --vhost-vsock-fd option is kept for compatibility, but it now just creates a /proc/self/fd path from the passed fd. Existing users are encouraged to migrate to --vhost-vsock-device. BUG=b:218223240 TEST=cargo test -p devices vsock::tests::params Change-Id: Ifad2b7ad0824d4f24d9b12a4af1448557fadcdc1 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3564224 Commit-Queue: Daniel Verkamp Tested-by: kokoro Reviewed-by: Alexandre Courbot Reviewed-by: Keiichi Watanabe --- devices/src/virtio/vhost/vsock.rs | 68 ++++++++----------------------- src/crosvm.rs | 3 +- src/linux/mod.rs | 7 +--- src/main.rs | 22 ++++------ 4 files changed, 28 insertions(+), 72 deletions(-) diff --git a/devices/src/virtio/vhost/vsock.rs b/devices/src/virtio/vhost/vsock.rs index 6ae6c641f3..832de5fda2 100644 --- a/devices/src/virtio/vhost/vsock.rs +++ b/devices/src/virtio/vhost/vsock.rs @@ -2,13 +2,13 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -use std::fs::{File, OpenOptions}; -use std::os::unix::prelude::{FromRawFd, OpenOptionsExt}; +use std::fs::OpenOptions; +use std::os::unix::prelude::OpenOptionsExt; use std::path::PathBuf; use std::thread; use anyhow::Context; -use base::{error, validate_raw_descriptor, warn, AsRawDescriptor, Event, RawDescriptor}; +use base::{error, open_file, warn, AsRawDescriptor, Event, RawDescriptor}; use data_model::{DataInit, Le64}; use serde::Deserialize; use vhost::Vhost; @@ -27,24 +27,10 @@ static VHOST_VSOCK_DEFAULT_PATH: &str = "/dev/vhost-vsock"; #[derive(Debug, Deserialize, PartialEq)] #[serde(deny_unknown_fields)] pub struct VhostVsockConfig { - #[serde(default)] - pub device: VhostVsockDeviceParameter, + pub device: Option, pub cid: u64, } -#[derive(Clone, Debug, Deserialize, PartialEq)] -#[serde(untagged)] -pub enum VhostVsockDeviceParameter { - Path(PathBuf), - Fd(RawDescriptor), -} - -impl Default for VhostVsockDeviceParameter { - fn default() -> Self { - VhostVsockDeviceParameter::Path(PathBuf::from(VHOST_VSOCK_DEFAULT_PATH)) - } -} - pub struct Vsock { worker_kill_evt: Option, kill_evt: Option, @@ -58,21 +44,17 @@ pub struct Vsock { impl Vsock { /// Create a new virtio-vsock device with the given VM cid. pub fn new(base_features: u64, vhost_config: &VhostVsockConfig) -> anyhow::Result { - let device_file = match &vhost_config.device { - VhostVsockDeviceParameter::Fd(fd) => { - let fd = validate_raw_descriptor(*fd) - .context("failed to validate fd for virtual socket device")?; - // Safe because the `fd` is actually owned by this process and - // we have a unique handle to it. - unsafe { File::from_raw_fd(fd) } - } - VhostVsockDeviceParameter::Path(path) => OpenOptions::new() + let device_file = open_file( + vhost_config + .device + .as_ref() + .unwrap_or(&PathBuf::from(VHOST_VSOCK_DEFAULT_PATH)), + OpenOptions::new() .read(true) .write(true) - .custom_flags(libc::O_CLOEXEC | libc::O_NONBLOCK) - .open(path) - .context("failed to open virtual socket device")?, - }; + .custom_flags(libc::O_CLOEXEC | libc::O_NONBLOCK), + ) + .context("failed to open virtual socket device")?; let kill_evt = Event::new().map_err(Error::CreateKillEvent)?; let handle = VhostVsockHandle::new(device_file); @@ -338,30 +320,12 @@ mod tests { #[test] fn params_from_key_values() { - // Fd device - let params = from_vsock_arg("device=42,cid=56").unwrap(); - assert_eq!( - params, - VhostVsockConfig { - device: VhostVsockDeviceParameter::Fd(42), - cid: 56, - } - ); - // No key for fd device - let params = from_vsock_arg("42,cid=56").unwrap(); - assert_eq!( - params, - VhostVsockConfig { - device: VhostVsockDeviceParameter::Fd(42), - cid: 56, - } - ); // Path device let params = from_vsock_arg("device=/some/path,cid=56").unwrap(); assert_eq!( params, VhostVsockConfig { - device: VhostVsockDeviceParameter::Path("/some/path".into()), + device: Some("/some/path".into()), cid: 56, } ); @@ -370,7 +334,7 @@ mod tests { assert_eq!( params, VhostVsockConfig { - device: VhostVsockDeviceParameter::Path("/some/path".into()), + device: Some("/some/path".into()), cid: 56, } ); @@ -379,7 +343,7 @@ mod tests { assert_eq!( params, VhostVsockConfig { - device: VhostVsockDeviceParameter::Path(VHOST_VSOCK_DEFAULT_PATH.into()), + device: None, cid: 56, } ); diff --git a/src/crosvm.rs b/src/crosvm.rs index b3c30b3a87..d21ab50063 100644 --- a/src/crosvm.rs +++ b/src/crosvm.rs @@ -28,7 +28,6 @@ use devices::virtio::cras_backend::Parameters as CrasSndParameters; use devices::virtio::fs::passthrough; #[cfg(feature = "gpu")] use devices::virtio::gpu::GpuParameters; -use devices::virtio::vhost::vsock::VhostVsockDeviceParameter; #[cfg(any(feature = "video-decoder", feature = "video-encoder"))] use devices::virtio::VideoBackendType; #[cfg(feature = "audio")] @@ -355,7 +354,7 @@ impl Default for JailConfig { /// Aggregate of all configurable options for a running VM. pub struct Config { pub kvm_device_path: PathBuf, - pub vhost_vsock_device: Option, + pub vhost_vsock_device: Option, pub vhost_net_device_path: PathBuf, pub vcpu_count: Option, pub vcpu_cgroup_path: Option, diff --git a/src/linux/mod.rs b/src/linux/mod.rs index 825ce7cac9..25d7becd65 100644 --- a/src/linux/mod.rs +++ b/src/linux/mod.rs @@ -23,7 +23,6 @@ use std::process; #[cfg(all(target_arch = "x86_64", feature = "gdb"))] use std::thread; -use devices::virtio::vhost::vsock::{VhostVsockConfig, VhostVsockDeviceParameter}; use libc; use acpi_tables::sdt::SDT; @@ -34,6 +33,7 @@ use base::{UnixSeqpacket, UnixSeqpacketListener, UnlinkUnixSeqpacketListener}; use devices::serial_device::SerialHardware; use devices::vfio::{VfioCommonSetup, VfioCommonTrait}; use devices::virtio::memory_mapper::MemoryMapperTrait; +use devices::virtio::vhost::vsock::VhostVsockConfig; #[cfg(feature = "gpu")] use devices::virtio::{self, EventDevice}; #[cfg(feature = "audio")] @@ -407,10 +407,7 @@ fn create_virtio_devices( if let Some(cid) = cfg.cid { let vhost_config = VhostVsockConfig { - device: cfg - .vhost_vsock_device - .clone() - .unwrap_or(VhostVsockDeviceParameter::default()), + device: cfg.vhost_vsock_device.clone(), cid, }; devs.push(create_vhost_vsock_device(cfg, &vhost_config)?); diff --git a/src/main.rs b/src/main.rs index 9a66e949b2..391c1dfe49 100644 --- a/src/main.rs +++ b/src/main.rs @@ -44,7 +44,6 @@ use devices::virtio::vhost::user::device::{ run_block_device, run_console_device, run_fs_device, run_net_device, run_vsock_device, run_wl_device, }; -use devices::virtio::vhost::vsock::VhostVsockDeviceParameter; #[cfg(any(feature = "video-decoder", feature = "video-encoder"))] use devices::virtio::VideoBackendType; #[cfg(feature = "gpu")] @@ -1109,17 +1108,14 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: expected: String::from("A vhost-vsock device was already specified"), }); } - cfg.vhost_vsock_device = Some(VhostVsockDeviceParameter::Fd( - value - .unwrap() - .parse() - .map_err(|_| argument::Error::InvalidValue { - value: value.unwrap().to_owned(), - expected: String::from( - "this value for `vhost-vsock-fd` needs to be integer", - ), - })?, - )); + let fd: i32 = value + .unwrap() + .parse() + .map_err(|_| argument::Error::InvalidValue { + value: value.unwrap().to_owned(), + expected: String::from("this value for `vhost-vsock-fd` needs to be integer"), + })?; + cfg.vhost_vsock_device = Some(PathBuf::from(format!("/proc/self/fd/{}", fd))); } "vhost-vsock-device" => { if cfg.vhost_vsock_device.is_some() { @@ -1136,7 +1132,7 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument:: }); } - cfg.vhost_vsock_device = Some(VhostVsockDeviceParameter::Path(vhost_vsock_device_path)); + cfg.vhost_vsock_device = Some(vhost_vsock_device_path); } "vhost-net-device" => { let vhost_net_device_path = PathBuf::from(value.unwrap());