From 8f6ff586a53a43f90658d08016505c2106a0435f Mon Sep 17 00:00:00 2001 From: FujiApple Date: Wed, 29 May 2024 18:16:54 +0800 Subject: [PATCH] feat: improve command line error reporting for durations (#1150) --- crates/trippy/src/config.rs | 67 +++++++++++++++++++++------------ crates/trippy/src/config/cmd.rs | 31 ++++++++------- 2 files changed, 60 insertions(+), 38 deletions(-) diff --git a/crates/trippy/src/config.rs b/crates/trippy/src/config.rs index 66bbdf34c..4e558d373 100644 --- a/crates/trippy/src/config.rs +++ b/crates/trippy/src/config.rs @@ -2,7 +2,6 @@ use anyhow::anyhow; use clap::ValueEnum; use clap_complete::Shell; use file::ConfigFile; -use humantime::format_duration; use itertools::Itertools; use serde::Deserialize; use std::collections::HashMap; @@ -413,13 +412,21 @@ impl TrippyConfig { let interface = cfg_layer_opt(args.interface, cfg_file_strategy.interface); let min_round_duration = cfg_layer( args.min_round_duration, - cfg_file_strategy.min_round_duration, - format_duration(defaults::DEFAULT_STRATEGY_MIN_ROUND_DURATION).to_string(), + cfg_file_strategy + .min_round_duration + .as_deref() + .map(humantime::parse_duration) + .transpose()?, + defaults::DEFAULT_STRATEGY_MIN_ROUND_DURATION, ); let max_round_duration = cfg_layer( args.max_round_duration, - cfg_file_strategy.max_round_duration, - format_duration(defaults::DEFAULT_STRATEGY_MAX_ROUND_DURATION).to_string(), + cfg_file_strategy + .max_round_duration + .as_deref() + .map(humantime::parse_duration) + .transpose()?, + defaults::DEFAULT_STRATEGY_MAX_ROUND_DURATION, ); let initial_sequence = cfg_layer( args.initial_sequence, @@ -433,8 +440,12 @@ impl TrippyConfig { ); let grace_duration = cfg_layer( args.grace_duration, - cfg_file_strategy.grace_duration, - format_duration(defaults::DEFAULT_STRATEGY_GRACE_DURATION).to_string(), + cfg_file_strategy + .grace_duration + .as_deref() + .map(humantime::parse_duration) + .transpose()?, + defaults::DEFAULT_STRATEGY_GRACE_DURATION, ); let max_inflight = cfg_layer( args.max_inflight, @@ -478,8 +489,12 @@ impl TrippyConfig { }; let read_timeout = cfg_layer( args.read_timeout, - cfg_file_strategy.read_timeout, - format_duration(defaults::DEFAULT_STRATEGY_READ_TIMEOUT).to_string(), + cfg_file_strategy + .read_timeout + .as_deref() + .map(humantime::parse_duration) + .transpose()?, + defaults::DEFAULT_STRATEGY_READ_TIMEOUT, ); let tui_max_samples = cfg_layer( args.tui_max_samples, @@ -498,8 +513,12 @@ impl TrippyConfig { ); let tui_refresh_rate = cfg_layer( args.tui_refresh_rate, - cfg_file_tui.tui_refresh_rate, - String::from(constants::DEFAULT_TUI_REFRESH_RATE), + cfg_file_tui + .tui_refresh_rate + .as_deref() + .map(humantime::parse_duration) + .transpose()?, + duration(constants::DEFAULT_TUI_REFRESH_RATE), ); let tui_privacy_max_ttl = cfg_layer( args.tui_privacy_max_ttl, @@ -545,8 +564,12 @@ impl TrippyConfig { ); let dns_timeout = cfg_layer( args.dns_timeout, - cfg_file_dns.dns_timeout, - String::from(constants::DEFAULT_DNS_TIMEOUT), + cfg_file_dns + .dns_timeout + .as_deref() + .map(humantime::parse_duration) + .transpose()?, + duration(constants::DEFAULT_DNS_TIMEOUT), ); let report_cycles = cfg_layer( args.report_cycles, @@ -559,10 +582,6 @@ impl TrippyConfig { (false, false, false, ProtocolConfig::Tcp) | (_, true, _, _) => Protocol::Tcp, (false, false, false, ProtocolConfig::Icmp) | (_, _, true, _) => Protocol::Icmp, }; - let read_timeout = humantime::parse_duration(&read_timeout)?; - let min_round_duration = humantime::parse_duration(&min_round_duration)?; - let max_round_duration = humantime::parse_duration(&max_round_duration)?; - let grace_duration = humantime::parse_duration(&grace_duration)?; let source_addr = source_address .as_ref() .map(|addr| { @@ -622,14 +641,12 @@ impl TrippyConfig { )); } }; - let tui_refresh_rate = humantime::parse_duration(&tui_refresh_rate)?; let dns_resolve_method = match dns_resolve_method_config { DnsResolveMethodConfig::System => ResolveMethod::System, DnsResolveMethodConfig::Resolv => ResolveMethod::Resolv, DnsResolveMethodConfig::Google => ResolveMethod::Google, DnsResolveMethodConfig::Cloudflare => ResolveMethod::Cloudflare, }; - let dns_timeout = humantime::parse_duration(&dns_timeout)?; let max_rounds = match mode { Mode::Stream | Mode::Tui => None, Mode::Pretty @@ -1280,11 +1297,11 @@ mod tests { #[test_case("trip example.com", Ok(cfg().min_round_duration(Duration::from_millis(1000)).build()); "default min round duration")] #[test_case("trip example.com --min-round-duration 250ms", Ok(cfg().min_round_duration(Duration::from_millis(250)).build()); "custom min round duration")] #[test_case("trip example.com -i 250ms", Ok(cfg().min_round_duration(Duration::from_millis(250)).build()); "custom min round duration short")] - #[test_case("trip example.com --min-round-duration 0", Err(anyhow!("time unit needed, for example 0sec or 0ms")); "invalid format min round duration")] + #[test_case("trip example.com --min-round-duration 0", Err(anyhow!("error: invalid value '0' for '--min-round-duration ': expected time unit (i.e. 100ms, 2s, 1000us) For more information, try '--help'.")); "invalid format min round duration")] #[test_case("trip example.com", Ok(cfg().min_round_duration(Duration::from_millis(1000)).build()); "default max round duration")] #[test_case("trip example.com --max-round-duration 1250ms", Ok(cfg().max_round_duration(Duration::from_millis(1250)).build()); "custom max round duration")] #[test_case("trip example.com -T 2s", Ok(cfg().max_round_duration(Duration::from_millis(2000)).build()); "custom max round duration short")] - #[test_case("trip example.com --min-round-duration 0", Err(anyhow!("time unit needed, for example 0sec or 0ms")); "invalid format max round duration")] + #[test_case("trip example.com --max-round-duration 0", Err(anyhow!("error: invalid value '0' for '--max-round-duration ': expected time unit (i.e. 100ms, 2s, 1000us) For more information, try '--help'.")); "invalid format max round duration")] #[test_case("trip example.com -i 250ms -T 250ms", Ok(cfg().min_round_duration(Duration::from_millis(250)).max_round_duration(Duration::from_millis(250)).build()); "custom min and max round duration")] #[test_case("trip example.com -i 300ms -T 250ms", Err(anyhow!("max-round-duration (250ms) must not be less than min-round-duration (300ms)")); "min round duration greater than max")] fn test_round_duration(cmd: &str, expected: anyhow::Result) { @@ -1294,7 +1311,7 @@ mod tests { #[test_case("trip example.com", Ok(cfg().grace_duration(Duration::from_millis(100)).build()); "default grace duration")] #[test_case("trip example.com --grace-duration 10ms", Ok(cfg().grace_duration(Duration::from_millis(10)).build()); "custom grace duration")] #[test_case("trip example.com -g 50ms", Ok(cfg().grace_duration(Duration::from_millis(50)).build()); "custom grace duration short")] - #[test_case("trip example.com --grace-duration 0", Err(anyhow!("time unit needed, for example 0sec or 0ms")); "invalid format grace duration")] + #[test_case("trip example.com --grace-duration 0", Err(anyhow!("error: invalid value '0' for '--grace-duration ': expected time unit (i.e. 100ms, 2s, 1000us) For more information, try '--help'.")); "invalid format grace duration")] #[test_case("trip example.com --grace-duration 9ms", Err(anyhow!("grace-duration (9ms) must be between 10ms and 1s inclusive")); "invalid low grace duration")] #[test_case("trip example.com --grace-duration 1001ms", Err(anyhow!("grace-duration (1.001s) must be between 10ms and 1s inclusive")); "invalid high grace duration")] fn test_grace_duration(cmd: &str, expected: anyhow::Result) { @@ -1337,7 +1354,7 @@ mod tests { #[test_case("trip example.com", Ok(cfg().read_timeout(Duration::from_millis(10)).build()); "default read timeout")] #[test_case("trip example.com --read-timeout 20ms", Ok(cfg().read_timeout(Duration::from_millis(20)).build()); "custom read timeout")] - #[test_case("trip example.com --read-timeout 20", Err(anyhow!("time unit needed, for example 20sec or 20ms")); "invalid custom read timeout")] + #[test_case("trip example.com --read-timeout 20", Err(anyhow!("error: invalid value '20' for '--read-timeout ': expected time unit (i.e. 100ms, 2s, 1000us) For more information, try '--help'.")); "invalid custom read timeout")] #[test_case("trip example.com --read-timeout 9ms", Err(anyhow!("read-timeout (9ms) must be between 10ms and 100ms inclusive")); "invalid low custom read timeout")] #[test_case("trip example.com --read-timeout 101ms", Err(anyhow!("read-timeout (101ms) must be between 10ms and 100ms inclusive")); "invalid high custom read timeout")] fn test_read_timeout(cmd: &str, expected: anyhow::Result) { @@ -1384,7 +1401,7 @@ mod tests { #[test_case("trip example.com", Ok(cfg().dns_timeout(Duration::from_millis(5000)).build()); "default dns timeout")] #[test_case("trip example.com --dns-timeout 20ms", Ok(cfg().dns_timeout(Duration::from_millis(20)).build()); "custom dns timeout")] - #[test_case("trip example.com --dns-timeout 20", Err(anyhow!("time unit needed, for example 20sec or 20ms")); "invalid custom dns timeout")] + #[test_case("trip example.com --dns-timeout 20", Err(anyhow!("error: invalid value '20' for '--dns-timeout ': expected time unit (i.e. 100ms, 2s, 1000us) For more information, try '--help'.")); "invalid custom dns timeout")] fn test_dns_timeout(cmd: &str, expected: anyhow::Result) { compare(parse_config(cmd), expected); } @@ -1440,7 +1457,7 @@ mod tests { #[test_case("trip example.com --tui-refresh-rate 200ms", Ok(cfg().tui_refresh_rate(Duration::from_millis(200)).build()); "custom tui refresh rate")] #[test_case("trip example.com --tui-refresh-rate 49ms", Err(anyhow!("tui-refresh-rate (49ms) must be between 50ms and 1s inclusive")); "invalid low tui refresh rate")] #[test_case("trip example.com --tui-refresh-rate 1001ms", Err(anyhow!("tui-refresh-rate (1.001s) must be between 50ms and 1s inclusive")); "invalid high tui refresh rate")] - #[test_case("trip example.com --tui-refresh-rate foo", Err(anyhow!("expected number at 0")); "invalid format tui refresh rate")] + #[test_case("trip example.com --tui-refresh-rate foo", Err(anyhow!("error: invalid value 'foo' for '--tui-refresh-rate ': expected time unit (i.e. 100ms, 2s, 1000us) For more information, try '--help'.")); "invalid format tui refresh rate")] fn test_tui_refresh_rate(cmd: &str, expected: anyhow::Result) { compare(parse_config(cmd), expected); } diff --git a/crates/trippy/src/config/cmd.rs b/crates/trippy/src/config/cmd.rs index 2d69be050..02ec7c572 100644 --- a/crates/trippy/src/config/cmd.rs +++ b/crates/trippy/src/config/cmd.rs @@ -5,10 +5,11 @@ use crate::config::{ LogFormat, LogSpanEvents, Mode, MultipathStrategyConfig, ProtocolConfig, TuiColor, TuiKeyBinding, }; -use anyhow::anyhow; +use anyhow::{anyhow, Context}; use clap::builder::Styles; use clap::Parser; use clap_complete::Shell; +use std::time::Duration; /// Trace a route to a host and record statistics #[allow(clippy::doc_markdown)] @@ -101,17 +102,17 @@ pub struct Args { pub interface: Option, /// The minimum duration of every round [default: 1s] - #[arg(short = 'i', long)] - pub min_round_duration: Option, + #[arg(short = 'i', long, value_parser = parse_duration)] + pub min_round_duration: Option, /// The maximum duration of every round [default: 1s] - #[arg(short = 'T', long)] - pub max_round_duration: Option, + #[arg(short = 'T', long, value_parser = parse_duration)] + pub max_round_duration: Option, /// The period of time to wait for additional ICMP responses after the target has responded /// [default: 100ms] - #[arg(short = 'g', long)] - pub grace_duration: Option, + #[arg(short = 'g', long, value_parser = parse_duration)] + pub grace_duration: Option, /// The initial sequence number [default: 33000] #[arg(long)] @@ -150,8 +151,8 @@ pub struct Args { pub icmp_extensions: bool, /// The socket read timeout [default: 10ms] - #[arg(long)] - pub read_timeout: Option, + #[arg(long, value_parser = parse_duration)] + pub read_timeout: Option, /// How to perform DNS queries [default: system] #[arg(value_enum, short = 'r', long)] @@ -162,8 +163,8 @@ pub struct Args { pub dns_resolve_all: bool, /// The maximum time to wait to perform DNS queries [default: 5s] - #[arg(long)] - pub dns_timeout: Option, + #[arg(long, value_parser = parse_duration)] + pub dns_timeout: Option, /// Lookup autonomous system (AS) information during DNS queries [default: false] #[arg(long, short = 'z')] @@ -206,8 +207,8 @@ pub struct Args { pub tui_preserve_screen: bool, /// The Tui refresh rate [default: 100ms] - #[arg(long)] - pub tui_refresh_rate: Option, + #[arg(long, value_parser = parse_duration)] + pub tui_refresh_rate: Option, /// The maximum ttl of hops which will be masked for privacy [default: 0] #[arg(long)] @@ -283,3 +284,7 @@ fn parse_tui_binding_value(value: &str) -> anyhow::Result<(TuiCommandItem, TuiKe let binding = TuiKeyBinding::try_from(&value[pos + 1..])?; Ok((item, binding)) } + +fn parse_duration(value: &str) -> anyhow::Result { + humantime::parse_duration(value).context("expected time unit (i.e. 100ms, 2s, 1000us)") +}