From e7fdf05f50aea214c2ac4dabcb309910e9d59fd0 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Sun, 21 Nov 2021 11:52:43 -0500
Subject: [PATCH] For every* config type, make defaults consistent.

This patch makes sure that for every* config type we have, the defaults
you get from a Builder match those you get from Serde, and that both
match the value that you get from arti_defaults.toml. Later down the
line I'll be adding some tests to keep these in sync.

* StorageConfig still has no defaults of its own, since we aren't so
sure we want other applications to use Arti's directories by default.
---
 crates/arti-client/src/config.rs  |  1 +
 crates/arti-config/src/options.rs | 17 +++++++++++++++
 crates/tor-circmgr/src/config.rs  | 35 ++++++++++++++++++++++++-------
 3 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/crates/arti-client/src/config.rs b/crates/arti-client/src/config.rs
index 4c77a87ab..5e5a57b6f 100644
--- a/crates/arti-client/src/config.rs
+++ b/crates/arti-client/src/config.rs
@@ -40,6 +40,7 @@ pub struct ClientAddrConfig {
     /// This option is off by default, since (by default) Tor exits will
     /// always reject connections to such addresses.
     #[builder(default)]
+    #[serde(default)]
     pub(crate) allow_local_addrs: bool,
 }
 
diff --git a/crates/arti-config/src/options.rs b/crates/arti-config/src/options.rs
index 9a9093530..3f925657e 100644
--- a/crates/arti-config/src/options.rs
+++ b/crates/arti-config/src/options.rs
@@ -25,13 +25,22 @@ pub struct LoggingConfig {
     ///
     /// Example: "info,tor_proto::channel=trace"
     // TODO(nickm) remove public elements when I revise this.
+    #[serde(default = "default_trace_filter")]
+    #[builder(default = "default_trace_filter()")]
     pub trace_filter: String,
 
     /// Whether to log to journald
     // TODO(nickm) remove public elements when I revise this.
+    #[serde(default)]
+    #[builder(default)]
     pub journald: bool,
 }
 
+/// Return a default value for `trace_filter`.
+fn default_trace_filter() -> String {
+    "debug".to_owned()
+}
+
 impl LoggingConfig {
     /// Return a new LoggingConfigBuilder
     pub fn builder() -> LoggingConfigBuilder {
@@ -56,9 +65,17 @@ impl From<LoggingConfig> for LoggingConfigBuilder {
 pub struct ProxyConfig {
     /// Port to listen on (at localhost) for incoming SOCKS
     /// connections.
+    #[serde(default = "default_socks_port")]
+    #[builder(default = "default_socks_port()")]
     socks_port: Option<u16>,
 }
 
+/// Return the default value for `socks_port`
+#[allow(clippy::unnecessary_wraps)]
+fn default_socks_port() -> Option<u16> {
+    Some(9150)
+}
+
 impl ProxyConfig {
     /// Return a new [`ProxyConfigBuilder`].
     pub fn builder() -> ProxyConfigBuilder {
diff --git a/crates/tor-circmgr/src/config.rs b/crates/tor-circmgr/src/config.rs
index 00f103222..58cefa4fc 100644
--- a/crates/tor-circmgr/src/config.rs
+++ b/crates/tor-circmgr/src/config.rs
@@ -85,31 +85,52 @@ impl From<PathConfig> for PathConfigBuilder {
 pub struct CircuitTiming {
     /// How long after a circuit has first been used should we give
     /// it out for new requests?
-    #[builder(default = "Duration::from_secs(60 * 10)")]
-    #[serde(with = "humantime_serde")]
+    #[builder(default = "default_max_dirtiness()")]
+    #[serde(with = "humantime_serde", default = "default_max_dirtiness")]
     pub(crate) max_dirtiness: Duration,
 
     /// When a circuit is requested, we stop retrying new circuits
     /// after this much time.
     // TODO: Impose a maximum or minimum?
-    #[builder(default = "Duration::from_secs(60)")]
-    #[serde(with = "humantime_serde")]
+    #[builder(default = "default_request_timeout()")]
+    #[serde(with = "humantime_serde", default = "default_request_timeout")]
     pub(crate) request_timeout: Duration,
 
     /// When a circuit is requested, we stop retrying new circuits after
     /// this many attempts.
     // TODO: Impose a maximum or minimum?
-    #[builder(default = "32")]
+    #[builder(default = "default_request_max_retries()")]
+    #[serde(default = "default_request_max_retries")]
     pub(crate) request_max_retries: u32,
 
     /// When waiting for requested circuits, wait at least this long
     /// before using a suitable-looking circuit launched by some other
     /// request.
-    #[builder(default = "Duration::from_millis(50)")]
-    #[serde(with = "humantime_serde")]
+    #[builder(default = "default_request_loyalty()")]
+    #[serde(with = "humantime_serde", default = "default_request_loyalty")]
     pub(crate) request_loyalty: Duration,
 }
 
+/// Return the default value for `max_dirtiness`.
+fn default_max_dirtiness() -> Duration {
+    Duration::from_secs(60 * 10)
+}
+
+/// Return the default value for `request_timeout`.
+fn default_request_timeout() -> Duration {
+    Duration::from_secs(60)
+}
+
+/// Return the default value for `request_max_retries`.
+fn default_request_max_retries() -> u32 {
+    32
+}
+
+/// Return the default request loyalty timeout.
+fn default_request_loyalty() -> Duration {
+    Duration::from_millis(50)
+}
+
 // NOTE: it seems that `unwrap` may be safe because of builder defaults
 // check `derive_builder` documentation for details
 // https://docs.rs/derive_builder/0.10.2/derive_builder/#default-values