Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vsock: Use PathBuf for socket paths instead of Strings #446

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 18 additions & 16 deletions vhost-device-gpio/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

use log::error;
use std::num::ParseIntError;
use std::path::PathBuf;
use std::process::exit;
use std::sync::{Arc, RwLock};
use std::thread::{spawn, JoinHandle};
Expand Down Expand Up @@ -58,7 +59,7 @@ Example, \"-c 4 -l 3:s4:6:s1\"\n";
struct GpioArgs {
/// Location of vhost-user Unix domain socket. This is suffixed by 0,1,2..socket_count-1.
#[clap(short, long)]
socket_path: String,
socket_path: PathBuf,

/// Number of guests (sockets) to connect to.
#[clap(short = 'c', long, default_value_t = 1)]
Expand Down Expand Up @@ -143,7 +144,7 @@ impl TryFrom<&str> for DeviceConfig {

#[derive(PartialEq, Debug)]
struct GpioConfiguration {
socket_path: String,
socket_path: PathBuf,
socket_count: usize,
devices: DeviceConfig,
}
Expand Down Expand Up @@ -173,7 +174,7 @@ impl TryFrom<GpioArgs> for GpioConfiguration {
}
}

fn start_device_backend<D: GpioDevice>(device: D, socket: String) -> Result<()> {
fn start_device_backend<D: GpioDevice>(device: D, socket: PathBuf) -> Result<()> {
let controller = GpioController::new(device).map_err(Error::CouldNotCreateGpioController)?;
let backend = Arc::new(RwLock::new(
VhostUserGpioBackend::new(controller).map_err(Error::CouldNotCreateBackend)?,
Expand All @@ -196,7 +197,7 @@ fn start_backend(args: GpioArgs) -> Result<()> {
let mut handles = Vec::new();

for i in 0..config.socket_count {
let socket = config.socket_path.to_owned() + &i.to_string();
let socket = config.socket_path.join(i.to_string());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path::join does not append the suffix to the file name component, but pushes a new component (e.g. "foo.sock0" vs "foo.sock/0")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess something like let socket = config.socket_path.with_extension(format!("sock{i}")); would work better? but that hardcodes the extension though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pub fn generate_socket_paths(&self) -> Vec<PathBuf> {
let socket_file_name = self
.socket_path
.file_name()
.expect("socket_path has no filename.");
let socket_file_parent = self
.socket_path
.parent()
.expect("socket_path has no parent directory.");
let make_socket_path = |i: usize| -> PathBuf {
let mut file_name = socket_file_name.to_os_string();
file_name.push(std::ffi::OsStr::new(&i.to_string()));
socket_file_parent.join(&file_name)
};
(0..self.socket_count).map(make_socket_path).collect()
}
}

let cfg = config.devices.inner[i];

let handle: JoinHandle<Result<()>> = spawn(move || loop {
Expand Down Expand Up @@ -258,9 +259,9 @@ mod tests {
}
}

fn get_cmd_args(path: &str, devices: &str, count: usize) -> GpioArgs {
fn get_cmd_args(path: PathBuf, devices: &str, count: usize) -> GpioArgs {
GpioArgs {
socket_path: path.to_string(),
socket_path: path,
socket_count: count,
device_list: devices.to_string(),
}
Expand All @@ -287,31 +288,31 @@ mod tests {

#[test]
fn test_gpio_parse_failure() {
let socket_name = "vgpio.sock";
let socket_name = PathBuf::from("vgpio.sock");

// Invalid device number
let cmd_args = get_cmd_args(socket_name, "1:4d:5", 3);
let cmd_args = get_cmd_args(socket_name.clone(), "1:4d:5", 3);
assert_matches!(
GpioConfiguration::try_from(cmd_args).unwrap_err(),
Error::ParseFailure(e) if e == "4d".parse::<u32>().unwrap_err()
);

// Zero socket count
let cmd_args = get_cmd_args(socket_name, "1:4", 0);
let cmd_args = get_cmd_args(socket_name.clone(), "1:4", 0);
assert_matches!(
GpioConfiguration::try_from(cmd_args).unwrap_err(),
Error::SocketCountInvalid(0)
);

// Duplicate client address: 4
let cmd_args = get_cmd_args(socket_name, "1:4:5:6:4", 5);
let cmd_args = get_cmd_args(socket_name.clone(), "1:4:5:6:4", 5);
assert_matches!(
GpioConfiguration::try_from(cmd_args).unwrap_err(),
Error::DeviceDuplicate(4)
);

// Device count mismatch
let cmd_args = get_cmd_args(socket_name, "1:4:5:6", 5);
let cmd_args = get_cmd_args(socket_name.clone(), "1:4:5:6", 5);
assert_matches!(
GpioConfiguration::try_from(cmd_args).unwrap_err(),
Error::DeviceCountMismatch(5, 4)
Expand All @@ -324,16 +325,16 @@ mod tests {

#[test]
fn test_gpio_parse_successful() {
let socket_name = "vgpio.sock";
let socket_name = PathBuf::from("vgpio.sock");

// Match expected and actual configuration
let cmd_args = get_cmd_args(socket_name, "1:4:32:21:5", 5);
let cmd_args = get_cmd_args(socket_name.clone(), "1:4:32:21:5", 5);
let config = GpioConfiguration::try_from(cmd_args).unwrap();

let expected_devices = DeviceConfig::new_with(vec![1, 4, 32, 21, 5]);
let expected_config = GpioConfiguration {
socket_count: 5,
socket_path: String::from(socket_name),
socket_path: socket_name,
devices: expected_devices,
};

Expand All @@ -342,8 +343,9 @@ mod tests {

#[test]
fn test_gpio_fail_listener_mock() {
let socket_name = "~/path/not/present/gpio";
let cmd_args = get_cmd_args(socket_name, "s1:s4:s3:s5", 4);
// This will fail the listeners and thread will panic.
let socket_name = PathBuf::from("~/path/not/present/gpio");
let cmd_args = get_cmd_args(socket_name, "1:4:3:5", 4);

assert_matches!(start_backend(cmd_args).unwrap_err(), Error::ServeFailed(_));
}
Expand Down
10 changes: 5 additions & 5 deletions vhost-device-rng/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,8 @@ mod tests {
period: VHU_RNG_MAX_PERIOD_MS,
max_bytes: usize::MAX,
socket_count: 1,
socket_path: "/some/socket_path".into(),
rng_source: "/dev/urandom".into(),
socket_path: PathBuf::from("/some/socket_path"),
rng_source: PathBuf::from("/dev/urandom"),
};

// All configuration elements should be what we expect them to be.
Expand Down Expand Up @@ -248,14 +248,14 @@ mod tests {
fn verify_start_backend() {
let dir = tempdir().unwrap();
let random_path = dir.path().join("urandom");
let _random_file = File::create(random_path.clone());
let _random_file = File::create(&random_path);

let mut config = VuRngConfig {
period_ms: 1000,
max_bytes: 512,
count: 1,
socket_path: "/invalid/path".into(),
rng_source: "/invalid/path".into(),
socket_path: PathBuf::from("/invalid/path"),
rng_source: PathBuf::from("/invalid/path"),
};

// An invalid RNG source file should trigger an AccessRngSourceFile error.
Expand Down
7 changes: 4 additions & 3 deletions vhost-device-scmi/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,13 +165,14 @@ mod tests {

#[test]
fn test_command_line() {
let path = "/foo/scmi.sock".to_owned();
let path = PathBuf::from("/foo/scmi.sock");
let params_string = format!(
"binary \
--device dummy \
-s {path} \
-s {} \
--device fake,name=foo,prop=value \
-d fake,name=bar"
-d fake,name=bar",
path.display()
);
let params: Vec<&str> = params_string.split_whitespace().collect();
let args: ScmiArgs = Parser::parse_from(params);
Expand Down
4 changes: 3 additions & 1 deletion vhost-device-scmi/src/vhu_scmi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,8 @@ impl VhostUserBackendMut for VuScmiBackend {

#[cfg(test)]
mod tests {
use std::path::PathBuf;

use virtio_bindings::virtio_ring::{VRING_DESC_F_NEXT, VRING_DESC_F_WRITE};
use virtio_queue::{mock::MockSplitQueue, Descriptor, Queue};
use vm_memory::{Address, GuestAddress, GuestMemoryAtomic, GuestMemoryMmap};
Expand Down Expand Up @@ -587,7 +589,7 @@ mod tests {

fn make_backend() -> VuScmiBackend {
let config = VuScmiConfig {
socket_path: "/foo/scmi.sock".into(),
socket_path: PathBuf::from("/foo/scmi.sock"),
devices: vec![],
};
VuScmiBackend::new(&config).unwrap()
Expand Down
12 changes: 4 additions & 8 deletions vhost-device-sound/src/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,7 @@ mod tests {
#[test]
fn test_sound_thread_success() {
crate::init_logger();
let config = SoundConfig::new(SOCKET_PATH.to_string(), false, BackendType::Null);
let config = SoundConfig::new(SOCKET_PATH.into(), false, BackendType::Null);

let chmaps = Arc::new(RwLock::new(vec![]));
let jacks = Arc::new(RwLock::new(vec![]));
Expand Down Expand Up @@ -821,7 +821,7 @@ mod tests {
#[test]
fn test_sound_thread_failure() {
crate::init_logger();
let config = SoundConfig::new(SOCKET_PATH.to_string(), false, BackendType::Null);
let config = SoundConfig::new(SOCKET_PATH.into(), false, BackendType::Null);

let chmaps = Arc::new(RwLock::new(vec![]));
let jacks = Arc::new(RwLock::new(vec![]));
Expand Down Expand Up @@ -902,7 +902,7 @@ mod tests {
fn test_sound_backend() {
crate::init_logger();
let test_dir = tempdir().expect("Could not create a temp test directory.");
let socket_path = test_dir.path().join(SOCKET_PATH).display().to_string();
let socket_path = test_dir.path().join(SOCKET_PATH);
let config = SoundConfig::new(socket_path, false, BackendType::Null);
let backend = VhostUserSoundBackend::new(config).expect("Could not create backend.");

Expand Down Expand Up @@ -980,11 +980,7 @@ mod tests {
crate::init_logger();
let test_dir = tempdir().expect("Could not create a temp test directory.");

let socket_path = test_dir
.path()
.join("sound_failures.socket")
.display()
.to_string();
let socket_path = test_dir.path().join("sound_failures.socket");
let config = SoundConfig::new(socket_path, false, BackendType::Null);
let backend = VhostUserSoundBackend::new(config);

Expand Down
13 changes: 7 additions & 6 deletions vhost-device-sound/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ use std::{
convert::TryFrom,
io::{Error as IoError, ErrorKind},
mem::size_of,
path::{Path, PathBuf},
sync::Arc,
};

Expand Down Expand Up @@ -255,7 +256,7 @@ impl TryFrom<Le32> for ControlMessageKind {
/// is allowed to configure the backend.
pub struct SoundConfig {
/// vhost-user Unix domain socket
socket: String,
socket: PathBuf,
/// use multiple threads to hanlde the virtqueues
multi_thread: bool,
/// audio backend variant
Expand All @@ -265,7 +266,7 @@ pub struct SoundConfig {
impl SoundConfig {
/// Create a new instance of the SoundConfig struct, containing the
/// parameters to be fed into the sound-backend server.
pub const fn new(socket: String, multi_thread: bool, audio_backend: BackendType) -> Self {
pub const fn new(socket: PathBuf, multi_thread: bool, audio_backend: BackendType) -> Self {
Self {
socket,
multi_thread,
Expand All @@ -275,8 +276,8 @@ impl SoundConfig {

/// Return the path of the unix domain socket which is listening to
/// requests from the guest.
pub fn get_socket_path(&self) -> String {
String::from(&self.socket)
pub fn get_socket_path(&self) -> &Path {
self.socket.as_path()
}

pub const fn get_audio_backend(&self) -> BackendType {
Expand Down Expand Up @@ -344,7 +345,7 @@ impl Drop for IOMessage {
/// vhost-device-sound backend server.
pub fn start_backend_server(config: SoundConfig) {
log::trace!("Using config {:?}.", &config);
let socket = config.get_socket_path();
let socket = config.get_socket_path().to_path_buf();
let backend = Arc::new(VhostUserSoundBackend::new(config).unwrap());

let mut daemon = VhostUserDaemon::new(
Expand Down Expand Up @@ -372,7 +373,7 @@ mod tests {
const SOCKET_PATH: &str = "vsound.socket";
crate::init_logger();

let config = SoundConfig::new(SOCKET_PATH.to_string(), false, BackendType::Null);
let config = SoundConfig::new(SOCKET_PATH.into(), false, BackendType::Null);

let backend = Arc::new(VhostUserSoundBackend::new(config).unwrap());
let daemon = VhostUserDaemon::new(
Expand Down
19 changes: 10 additions & 9 deletions vhost-device-sound/src/main.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Manos Pitsidianakis <[email protected]>
// Stefano Garzarella <[email protected]>
// SPDX-License-Identifier: Apache-2.0 or BSD-3-Clause
use std::convert::TryFrom;
use std::{convert::TryFrom, path::PathBuf};

use clap::Parser;
use vhost_device_sound::{start_backend_server, BackendType, Error, Result, SoundConfig};
Expand All @@ -11,7 +11,7 @@ use vhost_device_sound::{start_backend_server, BackendType, Error, Result, Sound
struct SoundArgs {
/// vhost-user Unix domain socket path.
#[clap(long)]
socket: String,
socket: PathBuf,
/// audio backend to be used
#[clap(long)]
#[clap(value_enum)]
Expand All @@ -22,9 +22,7 @@ impl TryFrom<SoundArgs> for SoundConfig {
type Error = Error;

fn try_from(cmd_args: SoundArgs) -> Result<Self> {
let socket = cmd_args.socket.trim().to_string();

Ok(SoundConfig::new(socket, false, cmd_args.backend))
Ok(SoundConfig::new(cmd_args.socket, false, cmd_args.backend))
}
}

Expand All @@ -45,9 +43,9 @@ mod tests {
use super::*;

impl SoundArgs {
fn from_args(socket: &str) -> Self {
fn from_args(socket: PathBuf) -> Self {
SoundArgs {
socket: socket.to_string(),
socket,
backend: BackendType::default(),
}
}
Expand All @@ -61,13 +59,16 @@ mod tests {
#[test]
fn test_sound_config_setup() {
init_logger();
let args = SoundArgs::from_args("/tmp/vhost-sound.socket");
let args = SoundArgs::from_args(PathBuf::from("/tmp/vhost-sound.socket"));

let config = SoundConfig::try_from(args);
assert!(config.is_ok());

let config = config.unwrap();
assert_eq!(config.get_socket_path(), "/tmp/vhost-sound.socket");
assert_eq!(
config.get_socket_path(),
PathBuf::from("/tmp/vhost-sound.socket")
);
}

#[rstest]
Expand Down
Loading