From 909775e2ca4ed8ec669299fd35ea299a1f595011 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 31 Jan 2025 16:48:41 -0500 Subject: [PATCH 1/5] Remove possible values from list of executables entirely If the possible values exist, they will appear in the usage message if the user enters a wrong executable value. To make them not appear at all there should be no list of possible values. A subsequent expression will verify that the name entered is a valid one. Signed-off-by: mulhern --- src/bin/stratisd-tools.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/bin/stratisd-tools.rs b/src/bin/stratisd-tools.rs index 5e49e3dd2f..a8265d7ace 100644 --- a/src/bin/stratisd-tools.rs +++ b/src/bin/stratisd-tools.rs @@ -49,9 +49,7 @@ fn main() { .arg( Arg::new("executable") .required(true) - .value_name("EXECUTABLE") - .value_parser(cmds().iter().map(|x| x.name()).collect::>()) - .hide_possible_values(true), + .value_name("EXECUTABLE"), ) .arg_required_else_help(true) .after_help(format!( From 41041196cd186521166336d2985e78bd249beaf8 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 31 Jan 2025 15:33:53 -0500 Subject: [PATCH 2/5] Make stratis-legacy-pool a command of stratisd-tools Signed-off-by: mulhern --- .githooks/pre-commit | 1 - .github/workflows/fedora.yml | 6 -- Cargo.toml | 8 +-- Makefile | 15 +---- plans/all.fmf | 5 +- src/bin/tools/cmds.rs | 66 ++++++++++++++++++- .../legacy_pool.rs} | 59 ++--------------- src/bin/tools/mod.rs | 1 + 8 files changed, 77 insertions(+), 84 deletions(-) rename src/bin/{stratis-legacy-pool.rs => tools/legacy_pool.rs} (60%) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 972cf61439..fd066b4582 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -5,7 +5,6 @@ export PROFILEDIR=debug make fmt-ci && make build && make stratisd-tools && - make build-test-extras && make build-min && make build-no-ipc && make test && diff --git a/.github/workflows/fedora.yml b/.github/workflows/fedora.yml index cc9a94688f..f3cb1927ac 100644 --- a/.github/workflows/fedora.yml +++ b/.github/workflows/fedora.yml @@ -45,9 +45,6 @@ jobs: - task: PROFILEDIR=debug make -f Makefile build toolchain: 1.84.0 # CURRENT DEVELOPMENT RUST TOOLCHAIN components: cargo - - task: PROFILEDIR=debug make -f Makefile build-test-extras - toolchain: 1.84.0 # CURRENT DEVELOPMENT RUST TOOLCHAIN - components: cargo - task: PROFILEDIR=debug make -f Makefile build-min toolchain: 1.84.0 # CURRENT DEVELOPMENT RUST TOOLCHAIN components: cargo @@ -72,9 +69,6 @@ jobs: - task: make -f Makefile build toolchain: 1.84.0 # CURRENT DEVELOPMENT RUST TOOLCHAIN components: cargo - - task: make -f Makefile build-test-extras - toolchain: 1.84.0 # CURRENT DEVELOPMENT RUST TOOLCHAIN - components: cargo - task: make -f Makefile build-min toolchain: 1.84.0 # CURRENT DEVELOPMENT RUST TOOLCHAIN components: cargo diff --git a/Cargo.toml b/Cargo.toml index 1edd4ef048..f908636149 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,10 +71,6 @@ required-features = ["udev_scripts"] name = "stratis-utils" required-features = ["engine"] -[[bin]] -name = "stratis-legacy-pool" -required-features = ["test_extras"] - [dependencies.async-trait] version = "0.1.51" optional = true @@ -295,11 +291,11 @@ engine = [ ] default = ["dbus_enabled", "engine"] dbus_enabled = ["dep:dbus", "dep:dbus-tree"] -extras = ["dep:pretty-hex"] +extras = ["dep:pretty-hex", "test_extras"] min = ["dep:termios"] systemd_compat = ["dep:bindgen"] udev_scripts = ["dep:data-encoding"] -test_extras = ["engine"] +test_extras = [] [lints.rust] warnings = { level = "deny" } diff --git a/Makefile b/Makefile index 425ec7a6cd..046590eb0b 100644 --- a/Makefile +++ b/Makefile @@ -51,7 +51,6 @@ MIN_FEATURES = --no-default-features --features engine,min NO_IPC_FEATURES = --no-default-features --features engine SYSTEMD_FEATURES = --no-default-features --features engine,min,systemd_compat EXTRAS_FEATURES = --no-default-features --features engine,extras,min -TEST_EXTRAS_FEATURES = --no-default-features --features test_extras UDEV_FEATURES = --no-default-features --features udev_scripts UTILS_FEATURES = --no-default-features --features engine,systemd_compat @@ -193,12 +192,6 @@ stratisd-tools: cargo ${BUILD} ${RELEASE_FLAG} \ --bin=stratisd-tools ${EXTRAS_FEATURES} ${TARGET_ARGS} -## Build the test extras -build-test-extras: - PKG_CONFIG_ALLOW_CROSS=1 \ - cargo build ${RELEASE_FLAG} \ - --bin=stratis-legacy-pool ${TEST_EXTRAS_FEATURES} ${TARGET_ARGS} - ## Build the stratis-dumpmetadata program ## Build stratis-min for early userspace stratis-min: @@ -421,12 +414,8 @@ clippy-utils: clippy-no-ipc: cargo clippy ${CLIPPY_OPTS} ${NO_IPC_FEATURES} -## Run clippy on no-ipc-build -clippy-test-extras: - cargo clippy ${CLIPPY_OPTS} ${TEST_EXTRAS_FEATURES} - ## Run clippy on the current source tree -clippy: clippy-macros clippy-min clippy-udev-utils clippy-no-ipc clippy-utils clippy-test-extras +clippy: clippy-macros clippy-min clippy-udev-utils clippy-no-ipc clippy-utils cargo clippy ${CLIPPY_OPTS} ## Lint Python parts of the source code @@ -443,7 +432,6 @@ lint: build-all-man build-all-rust build-min - build-test-extras build-udev-utils build-stratis-base32-decode build-stratis-str-cmp @@ -456,7 +444,6 @@ lint: clippy-macros clippy-min clippy-no-ipc - clippy-test-extras clippy-udev-utils docs-ci docs-rust diff --git a/plans/all.fmf b/plans/all.fmf index 0dbc96eb9f..be389cc0d0 100644 --- a/plans/all.fmf +++ b/plans/all.fmf @@ -50,11 +50,10 @@ execute: /python: prepare+: - - name: Build and install legacy pool script + - name: Make link for legacy pool utility how: shell script: - - PROFILEDIR=debug make build-test-extras - - mv target/debug/stratis-legacy-pool /usr/local/bin + - ln -s /usr/bin/stratisd-tools /usr/local/bin/stratis-legacy-pool discover+: filter: "tag:python" diff --git a/src/bin/tools/cmds.rs b/src/bin/tools/cmds.rs index 9fe84b43e4..5ca5f99541 100644 --- a/src/bin/tools/cmds.rs +++ b/src/bin/tools/cmds.rs @@ -4,9 +4,9 @@ use std::path::PathBuf; -use clap::{Arg, ArgAction, Command}; +use clap::{Arg, ArgAction, ArgGroup, Command}; -use crate::tools::{check_metadata, dump_metadata}; +use crate::tools::{check_metadata, dump_metadata, legacy_pool}; use stratisd::stratis::VERSION; @@ -147,10 +147,72 @@ impl<'a> ToolCommand<'a> for StratisPrintMetadata { } } +struct StratisLegacyPool; + +impl StratisLegacyPool { + fn cmd() -> Command { + Command::new("stratis-legacy-pool") + .version(VERSION) + .about("A script for facilitating testing; not to be used in production") + .arg(Arg::new("pool_name").num_args(1).required(true)) + .arg( + Arg::new("blockdevs") + .action(ArgAction::Append) + .value_parser(clap::value_parser!(PathBuf)) + .required(true), + ) + .arg( + Arg::new("key_desc") + .long("key-desc") + .num_args(1) + .required(false), + ) + .arg( + Arg::new("clevis") + .long("clevis") + .num_args(1) + .required(false) + .value_parser(["nbde", "tang", "tpm2"]) + .requires_if("nbde", "tang_args") + .requires_if("tang", "tang_args"), + ) + .arg( + Arg::new("tang_url") + .long("tang-url") + .num_args(1) + .required_if_eq("clevis", "nbde") + .required_if_eq("clevis", "tang"), + ) + .arg(Arg::new("thumbprint").long("thumbprint").num_args(1)) + .arg(Arg::new("trust_url").long("trust-url").num_args(0)) + .group( + ArgGroup::new("tang_args") + .arg("thumbprint") + .arg("trust_url"), + ) + } +} + +impl<'a> ToolCommand<'a> for StratisLegacyPool { + fn name(&self) -> &'a str { + "stratis-legacy-pool" + } + + fn run(&self, command_line_args: Vec) -> Result<(), String> { + let matches = StratisLegacyPool::cmd().get_matches_from(command_line_args); + legacy_pool::run(&matches).map_err(|err| format!("{err}")) + } + + fn show_in_after_help(&self) -> bool { + false + } +} + pub fn cmds<'a>() -> Vec>> { vec![ Box::new(StratisCheckMetadata), Box::new(StratisDumpMetadata), + Box::new(StratisLegacyPool), Box::new(StratisPrintMetadata), ] } diff --git a/src/bin/stratis-legacy-pool.rs b/src/bin/tools/legacy_pool.rs similarity index 60% rename from src/bin/stratis-legacy-pool.rs rename to src/bin/tools/legacy_pool.rs index fb5dbaeea0..03d2a4961f 100644 --- a/src/bin/stratis-legacy-pool.rs +++ b/src/bin/tools/legacy_pool.rs @@ -2,9 +2,9 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at http://mozilla.org/MPL/2.0/. -use std::{env, path::PathBuf}; +use std::path::PathBuf; -use clap::{Arg, ArgAction, ArgGroup, Command}; +use clap::ArgMatches; use serde_json::{json, Map, Value}; use stratisd::{ @@ -15,45 +15,6 @@ use stratisd::{ stratis::StratisResult, }; -fn stratis_legacy_pool_args() -> Command { - Command::new("stratis-legacy-pool") - .arg(Arg::new("pool_name").num_args(1).required(true)) - .arg( - Arg::new("blockdevs") - .action(ArgAction::Append) - .required(true), - ) - .arg( - Arg::new("key_desc") - .long("key-desc") - .num_args(1) - .required(false), - ) - .arg( - Arg::new("clevis") - .long("clevis") - .num_args(1) - .required(false) - .value_parser(["nbde", "tang", "tpm2"]) - .requires_if("nbde", "tang_args") - .requires_if("tang", "tang_args"), - ) - .arg( - Arg::new("tang_url") - .long("tang-url") - .num_args(1) - .required_if_eq("clevis", "nbde") - .required_if_eq("clevis", "tang"), - ) - .arg(Arg::new("thumbprint").long("thumbprint").num_args(1)) - .arg(Arg::new("trust_url").long("trust-url").num_args(0)) - .group( - ArgGroup::new("tang_args") - .arg("thumbprint") - .arg("trust_url"), - ) -} - type ParseReturn = StratisResult<( String, Vec, @@ -61,19 +22,15 @@ type ParseReturn = StratisResult<( Option<(String, Value)>, )>; -fn parse_args() -> ParseReturn { - let args = env::args().collect::>(); - let parser = stratis_legacy_pool_args(); - let matches = parser.get_matches_from(args); - +fn parse_args(matches: &ArgMatches) -> ParseReturn { let pool_name = matches .get_one::("pool_name") .expect("required") .clone(); let blockdevs = matches - .get_many::("blockdevs") + .get_many::("blockdevs") .expect("required") - .map(PathBuf::from) + .cloned() .collect::>(); let key_desc = match matches.get_one::("key_desc") { Some(kd) => Some(KeyDescription::try_from(kd)?), @@ -107,10 +64,8 @@ fn parse_args() -> ParseReturn { Ok((pool_name, blockdevs, key_desc, clevis_info)) } -fn main() -> StratisResult<()> { - env_logger::init(); - - let (name, devices, key_desc, clevis_info) = parse_args()?; +pub fn run(matches: &ArgMatches) -> StratisResult<()> { + let (name, devices, key_desc, clevis_info) = parse_args(matches)?; let unowned = ProcessedPathInfos::try_from( devices .iter() diff --git a/src/bin/tools/mod.rs b/src/bin/tools/mod.rs index a7d3a25f57..42fd600ce7 100644 --- a/src/bin/tools/mod.rs +++ b/src/bin/tools/mod.rs @@ -5,5 +5,6 @@ mod check_metadata; mod cmds; mod dump_metadata; +mod legacy_pool; pub use cmds::cmds; From 65db5674774a8e07183cb58036087c43e1428692 Mon Sep 17 00:00:00 2001 From: mulhern Date: Mon, 3 Feb 2025 17:58:14 -0500 Subject: [PATCH 3/5] Remove test_extras feature entirely It is possible to use the extras feature only, since it the test_extras code is always going to be included with the extras code. Signed-off-by: mulhern --- Cargo.toml | 3 +-- src/engine/mod.rs | 5 +---- src/engine/strat_engine/backstore/backstore/v1.rs | 2 +- src/engine/strat_engine/backstore/data_tier.rs | 2 +- src/engine/strat_engine/mod.rs | 8 ++++---- src/engine/strat_engine/pool/v1.rs | 4 ++-- src/engine/strat_engine/thinpool/thinpool.rs | 2 +- 7 files changed, 11 insertions(+), 15 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index f908636149..2a4e8f9f4b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -291,11 +291,10 @@ engine = [ ] default = ["dbus_enabled", "engine"] dbus_enabled = ["dep:dbus", "dep:dbus-tree"] -extras = ["dep:pretty-hex", "test_extras"] +extras = ["dep:pretty-hex"] min = ["dep:termios"] systemd_compat = ["dep:bindgen"] udev_scripts = ["dep:data-encoding"] -test_extras = [] [lints.rust] warnings = { level = "deny" } diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 2c07f2b5ec..eb4fc51682 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -2,11 +2,8 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at http://mozilla.org/MPL/2.0/. -#[cfg(feature = "test_extras")] -pub use self::strat_engine::{ProcessedPathInfos, StratPool}; - #[cfg(feature = "extras")] -pub use self::strat_engine::pool_inspection; +pub use self::strat_engine::{pool_inspection, ProcessedPathInfos, StratPool}; pub use self::{ engine::{BlockDev, Engine, Filesystem, KeyActions, Pool, Report}, diff --git a/src/engine/strat_engine/backstore/backstore/v1.rs b/src/engine/strat_engine/backstore/backstore/v1.rs index a9c5cb5a27..c33aecdd84 100644 --- a/src/engine/strat_engine/backstore/backstore/v1.rs +++ b/src/engine/strat_engine/backstore/backstore/v1.rs @@ -281,7 +281,7 @@ impl Backstore { /// be encrypted only with a kernel keyring and without Clevis information. /// /// WARNING: metadata changing event - #[cfg(any(test, feature = "test_extras"))] + #[cfg(any(test, feature = "extras"))] pub fn initialize( pool_name: Name, pool_uuid: PoolUuid, diff --git a/src/engine/strat_engine/backstore/data_tier.rs b/src/engine/strat_engine/backstore/data_tier.rs index cb24d3e65d..1e864353c3 100644 --- a/src/engine/strat_engine/backstore/data_tier.rs +++ b/src/engine/strat_engine/backstore/data_tier.rs @@ -49,7 +49,7 @@ impl DataTier { /// Initially 0 segments are allocated. /// /// WARNING: metadata changing event - #[cfg(any(test, feature = "test_extras"))] + #[cfg(any(test, feature = "extras"))] pub fn new(block_mgr: BlockDevMgr) -> DataTier { DataTier { block_mgr, diff --git a/src/engine/strat_engine/mod.rs b/src/engine/strat_engine/mod.rs index e9606ae9d7..a8e1b13d59 100644 --- a/src/engine/strat_engine/mod.rs +++ b/src/engine/strat_engine/mod.rs @@ -22,11 +22,11 @@ mod types; mod udev; mod writing; -#[cfg(feature = "test_extras")] -pub use self::{backstore::ProcessedPathInfos, pool::v1::StratPool}; - #[cfg(feature = "extras")] -pub use self::pool::inspection as pool_inspection; +pub use self::{ + backstore::ProcessedPathInfos, + pool::{inspection as pool_inspection, v1::StratPool}, +}; pub use self::{ backstore::integrity_meta_space, diff --git a/src/engine/strat_engine/pool/v1.rs b/src/engine/strat_engine/pool/v1.rs index 44fdb79473..d6f301664f 100644 --- a/src/engine/strat_engine/pool/v1.rs +++ b/src/engine/strat_engine/pool/v1.rs @@ -15,7 +15,7 @@ use serde_json::{Map, Value}; use devicemapper::{Bytes, DmNameBuf, Sectors}; use stratisd_proc_macros::strat_pool_impl_gen; -#[cfg(any(test, feature = "test_extras"))] +#[cfg(any(test, feature = "extras"))] use crate::engine::{ strat_engine::{ backstore::UnownedDevices, @@ -183,7 +183,7 @@ impl StratPool { /// 1. Initialize the block devices specified by paths. /// 2. Set up thinpool device to back filesystems. /// Precondition: p.is_absolute() is true for all p in paths - #[cfg(any(test, feature = "test_extras"))] + #[cfg(any(test, feature = "extras"))] pub fn initialize( name: &str, devices: UnownedDevices, diff --git a/src/engine/strat_engine/thinpool/thinpool.rs b/src/engine/strat_engine/thinpool/thinpool.rs index b7f2356986..ec351c2361 100644 --- a/src/engine/strat_engine/thinpool/thinpool.rs +++ b/src/engine/strat_engine/thinpool/thinpool.rs @@ -702,7 +702,7 @@ impl ThinPool { impl ThinPool { /// Make a new thin pool. - #[cfg(any(test, feature = "test_extras"))] + #[cfg(any(test, feature = "extras"))] pub fn new( pool_uuid: PoolUuid, thin_pool_size: &ThinPoolSizeParams, From 57d118dd363f1d37405610a48a967f16dfee0a39 Mon Sep 17 00:00:00 2001 From: mulhern Date: Mon, 3 Feb 2025 12:21:45 -0500 Subject: [PATCH 4/5] Add more explanatory text for stratis-legacy-pool Signed-off-by: mulhern --- src/bin/tools/cmds.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bin/tools/cmds.rs b/src/bin/tools/cmds.rs index 5ca5f99541..2daba35b31 100644 --- a/src/bin/tools/cmds.rs +++ b/src/bin/tools/cmds.rs @@ -153,7 +153,7 @@ impl StratisLegacyPool { fn cmd() -> Command { Command::new("stratis-legacy-pool") .version(VERSION) - .about("A script for facilitating testing; not to be used in production") + .about("A program for facilitating testing; not to be used in production. Creates a v1 pool equivalent to a pool that would be created by stratisd 3.7.3.") .arg(Arg::new("pool_name").num_args(1).required(true)) .arg( Arg::new("blockdevs") From cddb8d1716e3e94cd59ae5507ccb6fea5dbb1cbc Mon Sep 17 00:00:00 2001 From: mulhern Date: Mon, 3 Feb 2025 12:36:54 -0500 Subject: [PATCH 5/5] Add reinforcing prompt to stratis-legacy-pool Signed-off-by: mulhern --- src/bin/tools/legacy_pool.rs | 18 +++++++++++++++++- tests/client-dbus/tests/udev/_utils.py | 13 +++++++++---- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/bin/tools/legacy_pool.rs b/src/bin/tools/legacy_pool.rs index 03d2a4961f..d08067362f 100644 --- a/src/bin/tools/legacy_pool.rs +++ b/src/bin/tools/legacy_pool.rs @@ -2,7 +2,10 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at http://mozilla.org/MPL/2.0/. -use std::path::PathBuf; +use std::{ + io::{stdin, stdout, Write}, + path::PathBuf, +}; use clap::ArgMatches; use serde_json::{json, Map, Value}; @@ -66,6 +69,19 @@ fn parse_args(matches: &ArgMatches) -> ParseReturn { pub fn run(matches: &ArgMatches) -> StratisResult<()> { let (name, devices, key_desc, clevis_info) = parse_args(matches)?; + + println!("This program's purpose is to create v1 pools that can be used for testing. Under no circumstances should such pools be used in production."); + print!("Do you want to continue? [Y/n] "); + stdout().flush()?; + + let mut answer = String::new(); + stdin().read_line(&mut answer)?; + let answer = answer.trim_end(); + + if answer != "y" && answer != "Y" && answer != "yes" && answer != "Yes" { + return Ok(()); + } + let unowned = ProcessedPathInfos::try_from( devices .iter() diff --git a/tests/client-dbus/tests/udev/_utils.py b/tests/client-dbus/tests/udev/_utils.py index 9ea1784835..a5725c4281 100644 --- a/tests/client-dbus/tests/udev/_utils.py +++ b/tests/client-dbus/tests/udev/_utils.py @@ -112,11 +112,16 @@ def create_legacy_pool(): with subprocess.Popen( cmdline, text=True, - ) as output: - output.wait() - if output.returncode != 0: + stdin=subprocess.PIPE, + ) as process: + process.stdin.write( # pyright: ignore [reportOptionalMemberAccess] + f"Yes{os.linesep}" + ) + process.stdin.flush() # pyright: ignore [reportOptionalMemberAccess] + process.wait() + if process.returncode != 0: raise RuntimeError( - f"Unable to create a pool {name} with devices {devices}: {output.stderr}" + f"Unable to create a pool {name} with devices {devices}: {process.stderr}" ) newly_created = True