From a48bff27b1b7578e0f7ae1f2b3a457e28c226705 Mon Sep 17 00:00:00 2001 From: Albert Esteve Date: Tue, 29 Aug 2023 12:38:13 +0200 Subject: [PATCH 1/4] sound: cli backend selector Change the CLI backend option to receive values listed in a ValueEnum. Current '--help' output: ``` A virtio-sound device using the vhost-user protocol. Usage: vhost-user-sound --socket --backend Options: --socket vhost-user Unix domain socket path --backend audio backend to be used [possible values: null, pipewire, alsa] -h, --help Print help -V, --version Print version ``` If a wrong backend is given, it give hints: ``` $ cargo run -- --socket /tmp/sound.sock --backend nul error: invalid value 'nul' for '' [possible values: null, pipewire, alsa] tip: a similar value exists: 'null' ``` Signed-off-by: Albert Esteve --- crates/sound/README.md | 2 +- crates/sound/src/audio_backends.rs | 20 +++++++++++++------- crates/sound/src/device.rs | 2 +- crates/sound/src/lib.rs | 17 +++++++++++++---- crates/sound/src/main.rs | 12 ++++++------ 5 files changed, 34 insertions(+), 19 deletions(-) diff --git a/crates/sound/README.md b/crates/sound/README.md index faea312..52ad5ad 100644 --- a/crates/sound/README.md +++ b/crates/sound/README.md @@ -16,7 +16,7 @@ generated with help2man target/debug/vhost-user-sound |mandoc vhost-user Unix domain socket path --backend - audio backend to be used (supported: null) + audio backend to be used [possible values: null, pipewire, alsa] -h, --help Print help diff --git a/crates/sound/src/audio_backends.rs b/crates/sound/src/audio_backends.rs index 950cd3f..56b5280 100644 --- a/crates/sound/src/audio_backends.rs +++ b/crates/sound/src/audio_backends.rs @@ -17,7 +17,7 @@ use self::alsa::AlsaBackend; use self::null::NullBackend; #[cfg(feature = "pw-backend")] use self::pipewire::PwBackend; -use crate::{device::ControlMessage, stream::Stream, Error, Result}; +use crate::{device::ControlMessage, stream::Stream, BackendType, Error, Result}; pub trait AudioBackend { fn write(&self, stream_id: u32) -> Result<()>; @@ -46,17 +46,23 @@ pub trait AudioBackend { } pub fn alloc_audio_backend( - name: String, + backend: BackendType, streams: Arc>>, ) -> Result> { - log::trace!("allocating audio backend {}", name); - match name.as_str() { + log::trace!("allocating audio backend {:?}", backend); + match backend { #[cfg(feature = "null-backend")] - "null" => Ok(Box::new(NullBackend::new(streams))), + BackendType::Null => Ok(Box::new(NullBackend::new(streams))), #[cfg(feature = "pw-backend")] - "pipewire" => Ok(Box::new(PwBackend::new(streams))), + BackendType::Pipewire => Ok(Box::new(PwBackend::new(streams))), #[cfg(feature = "alsa-backend")] - "alsa" => Ok(Box::new(AlsaBackend::new(streams))), + BackendType::Alsa => Ok(Box::new(AlsaBackend::new(streams))), + // By default all features are enabled and this branch is unreachable. + // Nonetheless, it is required when inidividual features (or no features + // at all) are enabled. + // To avoid having a complicated compilation condition and make the + // code more maintainable, we supress the unreachable_patterns warning. + #[allow(unreachable_patterns)] _ => Err(Error::AudioBackendNotSupported), } } diff --git a/crates/sound/src/device.rs b/crates/sound/src/device.rs index c2f40c2..314e55b 100644 --- a/crates/sound/src/device.rs +++ b/crates/sound/src/device.rs @@ -557,7 +557,7 @@ impl VhostUserSoundBackend { )?)] }; - let audio_backend = alloc_audio_backend(config.audio_backend_name, streams)?; + let audio_backend = alloc_audio_backend(config.audio_backend, streams)?; Ok(Self { threads, diff --git a/crates/sound/src/lib.rs b/crates/sound/src/lib.rs index 474add1..2ad4dde 100644 --- a/crates/sound/src/lib.rs +++ b/crates/sound/src/lib.rs @@ -12,6 +12,7 @@ use std::{ sync::Arc, }; +use clap::ValueEnum; use log::{info, warn}; pub use stream::Stream; use thiserror::Error as ThisError; @@ -90,6 +91,14 @@ impl From for Error { } } +#[derive(ValueEnum, Clone, Default, Debug)] +pub enum BackendType { + #[default] + Null, + Pipewire, + Alsa, +} + #[derive(Debug)] pub struct InvalidControlMessage(u32); @@ -203,18 +212,18 @@ pub struct SoundConfig { socket: String, /// use multiple threads to hanlde the virtqueues multi_thread: bool, - /// audio backend name - audio_backend_name: String, + /// audio backend variant + audio_backend: BackendType, } impl SoundConfig { /// Create a new instance of the SoundConfig struct, containing the /// parameters to be fed into the sound-backend server. - pub fn new(socket: String, multi_thread: bool, audio_backend_name: String) -> Self { + pub fn new(socket: String, multi_thread: bool, audio_backend: BackendType) -> Self { Self { socket, multi_thread, - audio_backend_name, + audio_backend, } } diff --git a/crates/sound/src/main.rs b/crates/sound/src/main.rs index 11bed61..8f2c627 100644 --- a/crates/sound/src/main.rs +++ b/crates/sound/src/main.rs @@ -4,7 +4,7 @@ use std::convert::TryFrom; use clap::Parser; -use vhost_user_sound::{start_backend_server, Error, Result, SoundConfig}; +use vhost_user_sound::{start_backend_server, BackendType, Error, Result, SoundConfig}; #[derive(Parser, Debug)] #[clap(version, about, long_about = None)] @@ -12,9 +12,10 @@ struct SoundArgs { /// vhost-user Unix domain socket path. #[clap(long)] socket: String, - /// audio backend to be used (supported: null) + /// audio backend to be used #[clap(long)] - backend: String, + #[clap(value_enum)] + backend: BackendType, } impl TryFrom for SoundConfig { @@ -22,9 +23,8 @@ impl TryFrom for SoundConfig { fn try_from(cmd_args: SoundArgs) -> Result { let socket = cmd_args.socket.trim().to_string(); - let backend = cmd_args.backend.trim().to_string(); - Ok(SoundConfig::new(socket, false, backend)) + Ok(SoundConfig::new(socket, false, cmd_args.backend)) } } @@ -48,7 +48,7 @@ mod tests { fn from_args(socket: &str) -> Self { SoundArgs { socket: socket.to_string(), - backend: "null".to_string(), + backend: BackendType::default(), } } } From 6c5b2db7db824df316161a17131f6ac960013248 Mon Sep 17 00:00:00 2001 From: Albert Esteve Date: Thu, 31 Aug 2023 11:19:44 +0200 Subject: [PATCH 2/4] sound: audio backend allow unused When compiled with no features, the `streams` parameter of the alloc_audio_backend() function is not used. Mark it as allowed to avoid the compilation warning. Signed-off-by: Albert Esteve --- crates/sound/src/audio_backends.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/sound/src/audio_backends.rs b/crates/sound/src/audio_backends.rs index 56b5280..9319bac 100644 --- a/crates/sound/src/audio_backends.rs +++ b/crates/sound/src/audio_backends.rs @@ -47,7 +47,8 @@ pub trait AudioBackend { pub fn alloc_audio_backend( backend: BackendType, - streams: Arc>>, + // Unused when compiled with no features. + #[allow(unused_variables)] streams: Arc>>, ) -> Result> { log::trace!("allocating audio backend {:?}", backend); match backend { From 4867edc009af4a2db9e51b5e715bf66fa78fd529 Mon Sep 17 00:00:00 2001 From: Albert Esteve Date: Tue, 29 Aug 2023 13:12:46 +0200 Subject: [PATCH 3/4] sound: test cli backend argument Signed-off-by: Albert Esteve --- crates/sound/src/lib.rs | 6 +++++- crates/sound/src/main.rs | 20 +++++++++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/crates/sound/src/lib.rs b/crates/sound/src/lib.rs index 2ad4dde..23fed0a 100644 --- a/crates/sound/src/lib.rs +++ b/crates/sound/src/lib.rs @@ -91,7 +91,7 @@ impl From for Error { } } -#[derive(ValueEnum, Clone, Default, Debug)] +#[derive(ValueEnum, Clone, Copy, Default, Debug, Eq, PartialEq)] pub enum BackendType { #[default] Null, @@ -232,6 +232,10 @@ impl SoundConfig { pub fn get_socket_path(&self) -> String { String::from(&self.socket) } + + pub fn get_audio_backend(&self) -> BackendType { + self.audio_backend + } } pub struct IOMessage { diff --git a/crates/sound/src/main.rs b/crates/sound/src/main.rs index 8f2c627..0d52321 100644 --- a/crates/sound/src/main.rs +++ b/crates/sound/src/main.rs @@ -55,7 +55,7 @@ mod tests { #[test] #[serial] - fn test_vsock_config_setup() { + fn test_sound_config_setup() { let args = SoundArgs::from_args("/tmp/vhost-sound.socket"); let config = SoundConfig::try_from(args); @@ -64,4 +64,22 @@ mod tests { let config = config.unwrap(); assert_eq!(config.get_socket_path(), "/tmp/vhost-sound.socket"); } + + #[test] + #[serial] + fn test_cli_backend_arg() { + let args: SoundArgs = Parser::parse_from([ + "", + "--socket", + "/tmp/vhost-sound.socket ", + "--backend", + "null", + ]); + + let config = SoundConfig::try_from(args); + assert!(config.is_ok()); + + let config = config.unwrap(); + assert_eq!(config.get_audio_backend(), BackendType::Null); + } } From cd955a71324d888d33b670f7c6a0cca278d76047 Mon Sep 17 00:00:00 2001 From: Albert Esteve Date: Tue, 29 Aug 2023 13:20:38 +0200 Subject: [PATCH 4/4] sound: introduce rstest crate Introduce 'rstest'[1] crate dependency to allow run multiple parametrized tests (and fixtures). [1] - https://docs.rs/rstest/latest/rstest/ Signed-off-by: Albert Esteve --- Cargo.lock | 57 ++++++++++++++++++++++++++++++++++++++++ crates/sound/Cargo.toml | 1 + crates/sound/src/main.rs | 12 ++++++--- 3 files changed, 66 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 76f9d89..5007d0c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -412,6 +412,12 @@ version = "0.3.28" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "76d3d132be6c0e6aa1534069c705a74a5997a356c0dc2f86a47765e5617c5b65" +[[package]] +name = "futures-timer" +version = "3.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e64b03909df88034c26dc1547e8970b91f98bdb65165d6a4e9110d94263dbb2c" + [[package]] name = "futures-util" version = "0.3.28" @@ -868,12 +874,56 @@ version = "0.7.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e5ea92a5b6195c6ef2a0295ea818b312502c6fc94dde986c5553242e18fd4ce2" +[[package]] +name = "relative-path" +version = "1.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c707298afce11da2efef2f600116fa93ffa7a032b5d7b628aa17711ec81383ca" + +[[package]] +name = "rstest" +version = "0.18.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "97eeab2f3c0a199bc4be135c36c924b6590b88c377d416494288c14f2db30199" +dependencies = [ + "futures", + "futures-timer", + "rstest_macros", + "rustc_version", +] + +[[package]] +name = "rstest_macros" +version = "0.18.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d428f8247852f894ee1be110b375111b586d4fa431f6c46e64ba5a0dcccbe605" +dependencies = [ + "cfg-if", + "glob", + "proc-macro2", + "quote", + "regex", + "relative-path", + "rustc_version", + "syn 2.0.26", + "unicode-ident", +] + [[package]] name = "rustc-hash" version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "08d43f7aa6b08d49f382cde6a7982047c3426db949b1424bc4b7ec9ae12c6ce2" +[[package]] +name = "rustc_version" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bfa0f585226d2e68097d4f95d113b15b83a82e819ab25717ec0590d9584ef366" +dependencies = [ + "semver", +] + [[package]] name = "rustix" version = "0.37.23" @@ -907,6 +957,12 @@ version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" +[[package]] +name = "semver" +version = "1.0.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b0293b4b29daaf487284529cc2f5675b8e57c61f70167ba415a463651fd6a918" + [[package]] name = "serde" version = "1.0.163" @@ -1211,6 +1267,7 @@ dependencies = [ "log", "pipewire", "pipewire-sys", + "rstest", "serial_test", "thiserror", "vhost", diff --git a/crates/sound/Cargo.toml b/crates/sound/Cargo.toml index 962fe01..55a6a6d 100644 --- a/crates/sound/Cargo.toml +++ b/crates/sound/Cargo.toml @@ -35,3 +35,4 @@ vmm-sys-util = "0.11" [dev-dependencies] serial_test = "1.0" +rstest = "0.18.2" diff --git a/crates/sound/src/main.rs b/crates/sound/src/main.rs index 0d52321..f4dcced 100644 --- a/crates/sound/src/main.rs +++ b/crates/sound/src/main.rs @@ -43,6 +43,7 @@ mod tests { use serial_test::serial; use super::*; + use rstest::*; impl SoundArgs { fn from_args(socket: &str) -> Self { @@ -65,21 +66,24 @@ mod tests { assert_eq!(config.get_socket_path(), "/tmp/vhost-sound.socket"); } - #[test] + #[rstest] #[serial] - fn test_cli_backend_arg() { + #[case::null_backend("null", BackendType::Null)] + #[case::pipewire("pipewire", BackendType::Pipewire)] + #[case::alsa("alsa", BackendType::Alsa)] + fn test_cli_backend_arg(#[case] backend_name: &str, #[case] backend: BackendType) { let args: SoundArgs = Parser::parse_from([ "", "--socket", "/tmp/vhost-sound.socket ", "--backend", - "null", + backend_name, ]); let config = SoundConfig::try_from(args); assert!(config.is_ok()); let config = config.unwrap(); - assert_eq!(config.get_audio_backend(), BackendType::Null); + assert_eq!(config.get_audio_backend(), backend); } }