Merge branch 'socket-addr-list-builder' into 'main'

FallbackDir: orports: Introduce and use VecBuilder

See merge request tpo/core/arti!474

Cargo.lock

@@ -3336,6 +3336,7 @@ dependencies = [
  "derive_builder",
  "directories",
  "dirs",
+ "educe",
  "once_cell",
  "paste",
  "rmp-serde",

crates/arti-client/src/config.rs

@@ -389,11 +389,12 @@ mod test {
             .name("Fred")
             .v3ident([22; 20].into())
             .clone();
-        let fallback = dir::FallbackDir::builder()
+        let mut fallback = dir::FallbackDir::builder();
+        fallback
             .rsa_identity([23; 20].into())
             .ed_identity([99; 32].into())
-            .orports(vec!["127.0.0.7:7".parse().unwrap()])
-            .clone();
+            .orports()
+            .push("127.0.0.7:7".parse().unwrap());
 
         let mut bld = TorClientConfig::builder();
         bld.tor_network().set_authorities(vec![auth]);

crates/arti/src/cfg.rs

@@ -226,11 +226,12 @@ mod test {
             .name("Fred")
             .v3ident([22; 20].into())
             .clone();
-        let fallback = dir::FallbackDir::builder()
+        let mut fallback = dir::FallbackDir::builder();
+        fallback
             .rsa_identity([23; 20].into())
             .ed_identity([99; 32].into())
-            .orports(vec!["127.0.0.7:7".parse().unwrap()])
-            .clone();
+            .orports()
+            .push("127.0.0.7:7".parse().unwrap());
 
         let mut bld = ArtiConfig::builder();
         bld.proxy().socks_port(Some(9999));

crates/tor-circmgr/src/path/dirpath.rs

@@ -117,18 +117,22 @@ mod test {
     #[test]
     fn dirpath_fallback() {
         let fb_owned = vec![
-            FallbackDir::builder()
-                .rsa_identity([0x01; 20].into())
-                .ed_identity([0x01; 32].into())
-                .orport("127.0.0.1:9000".parse().unwrap())
-                .build()
-                .unwrap(),
-            FallbackDir::builder()
-                .rsa_identity([0x03; 20].into())
-                .ed_identity([0x03; 32].into())
-                .orport("127.0.0.1:9003".parse().unwrap())
-                .build()
-                .unwrap(),
+            {
+                let mut bld = FallbackDir::builder();
+                bld.rsa_identity([0x01; 20].into())
+                    .ed_identity([0x01; 32].into())
+                    .orports()
+                    .push("127.0.0.1:9000".parse().unwrap());
+                bld.build().unwrap()
+            },
+            {
+                let mut bld = FallbackDir::builder();
+                bld.rsa_identity([0x03; 20].into())
+                    .ed_identity([0x03; 32].into())
+                    .orports()
+                    .push("127.0.0.1:9003".parse().unwrap());
+                bld.build().unwrap()
+            },
         ];
         let fb: FallbackList = fb_owned.clone().into();
         let dirinfo = (&fb).into();

crates/tor-config/Cargo.toml

@@ -21,6 +21,7 @@ tor-basic-utils = { path="../tor-basic-utils", version = "0.2.0" }
 
 thiserror = "1"
 derive_builder = { version = "0.11.2", git = "https://github.com/ijackson/rust-derive-builder", rev = "ba0c1a5311bd9f93ddf5f5b8ec2a5f6f03b22fbe" }
+educe = "0.4.6"
 once_cell = "1"
 paste = "1"
 serde = { version = "1.0.103", features = ["derive"] }

crates/tor-config/src/lib.rs

@@ -51,6 +51,7 @@ pub mod list_builder;
 mod mut_cfg;
 mod path;
 
+pub use educe;
 pub use err::{ConfigBuildError, ReconfigureError};
 pub use mut_cfg::MutCfg;
 pub use paste::paste;

crates/tor-config/src/list_builder.rs

@@ -151,7 +151,9 @@
 /// `#[derive(Default, Clone, Deserialize)]` will be applied to the generated builder,
 /// but you can specify other attributes too.
 /// There is no need to supply any documentation; this is an internal struct and
-/// the macro will supply a suitable (generic) doc comment.
+/// the macro will supply a suitable (bland) doc comment.
+/// (If you do supply documentation, the autogenerated docs will be appended,
+/// so start with a summary line.)
 /// Documentation for the semantics and default value should be applied
 /// to the field(s) in the containing struct(s).
 ///
@@ -172,29 +174,51 @@
 /// The `item_build` clause, if supplied, provides a closure with type
 /// `FnMut(&ThingBuilder) -> Result<Thing, ConfigBuildError>`;
 /// the default is to call `thing_builder.build()`.
+///
+/// `[$generics]` are generics for `$ListBuilder`.
+/// Inline bounds (`T: Debug`) are not supported; use a `where` clause instead.
+/// Due to limitations of `macro_rules`, the parameters must be within `[ ]` rather than `< >`,
+/// and an extraneous pair of `[ ]` must appear around any `$where_clauses`.
+//
+// This difficulty with macro_rules is not well documented.
+// The upstream Rust bug tracker has this issue
+//   https://github.com/rust-lang/rust/issues/73174
+//   Matching function signature is nearly impossible in declarative macros (mbe)
+// which is not precisely this problem but is very nearby.
+// There's also the vapourware "declarative macros 2.0"
+//   https://github.com/rust-lang/rust/issues/39412
 #[macro_export]
 macro_rules! define_list_builder_helper {
     {
         $(#[ $docs_and_attrs:meta ])*
-        $vis:vis struct $ListBuilder:ident {
+        $vis:vis
+        struct $ListBuilder:ident $( [ $($generics:tt)* ] )?
+        $( where [ $($where_clauses:tt)* ] )?
+        {
             $field_vis:vis $things:ident : [$EntryBuilder:ty] $(,)?
         }
         built: $Built:ty = $built:expr;
         default = $default:expr;
         $( item_build: $item_build:expr; )?
     } => {
-        #[derive(Default, Clone, $crate::serde::Deserialize)]
+        #[derive($crate::educe::Educe, Clone, $crate::serde::Deserialize)]
+        #[educe(Default)]
         #[serde(transparent)]
+        $(#[ $docs_and_attrs ])*
         /// Wrapper struct to help derive_builder find the right types and methods
         ///
         /// This struct is not part of the configuration API.
         /// Refer to the containing structures for information on how to build the config.
-        $vis struct $ListBuilder {
+        $vis struct $ListBuilder $( < $($generics)* > )?
+        $( where $($where_clauses)* )?
+        {
             /// The list, as overridden
             $field_vis $things: Option<Vec<$EntryBuilder>>,
         }
 
-        impl $ListBuilder {
+        impl $( < $($generics)* > )? $ListBuilder $( < $($generics)* > )?
+        $( where $($where_clauses)* )?
+        {
             /// Resolve this list to a list of built items.
             ///
             /// If the value is still the [`Default`],
@@ -307,6 +331,47 @@ macro_rules! define_list_builder_accessors {
     }
 }
 
+define_list_builder_helper! {
+    /// List of `T`, a straightforward type, being built as part of the configuration
+    ///
+    /// The default is the empty list.
+    ///
+    /// ### Example
+    ///
+    /// ```
+    /// use derive_builder::Builder;
+    /// use serde::Deserialize;
+    /// use tor_config::{ConfigBuildError};
+    /// use tor_config::{define_list_builder_accessors, list_builder::VecBuilder};
+    /// use std::net::SocketAddr;
+    ///
+    /// #[derive(Debug, Clone, Builder)]
+    /// #[builder(build_fn(error = "ConfigBuildError"))]
+    /// #[builder(derive(Deserialize))]
+    /// pub struct FallbackDir {
+    ///     #[builder(sub_builder(fn_name = "build"), setter(custom))]
+    ///     orports: Vec<SocketAddr>,
+    /// }
+    ///
+    /// define_list_builder_accessors! {
+    ///     struct FallbackDirBuilder {
+    ///         pub orports: [SocketAddr],
+    ///     }
+    /// }
+    ///
+    /// let mut bld = FallbackDirBuilder::default();
+    /// bld.orports().push("[2001:db8:0::42]:12".parse().unwrap());
+    /// assert_eq!( bld.build().unwrap().orports[0].to_string(),
+    ///             "[2001:db8::42]:12" );
+    /// ```
+    pub struct VecBuilder[T] where [T: Clone] {
+        values: [T],
+    }
+    built: Vec<T> = values;
+    default = vec![];
+    item_build: |item| Ok(item.clone());
+}
+
 #[cfg(test)]
 mod test {
     use derive_builder::Builder;

crates/tor-dirmgr/src/config.rs

@@ -333,12 +333,14 @@ mod test {
         ]);
         assert!(bld.build().is_err());
 
-        bld.set_fallback_caches(vec![FallbackDir::builder()
-            .rsa_identity([b'x'; 20].into())
-            .ed_identity([b'y'; 32].into())
-            .orport("127.0.0.1:99".parse().unwrap())
-            .orport("[::]:99".parse().unwrap())
-            .clone()]);
+        bld.set_fallback_caches(vec![{
+            let mut bld = FallbackDir::builder();
+            bld.rsa_identity([b'x'; 20].into())
+                .ed_identity([b'y'; 32].into());
+            bld.orports().push("127.0.0.1:99".parse().unwrap());
+            bld.orports().push("[::]:99".parse().unwrap());
+            bld
+        }]);
         let cfg = bld.build().unwrap();
         assert_eq!(cfg.authorities.len(), 2);
         assert_eq!(cfg.fallback_caches.len(), 1);

crates/tor-guardmgr/src/fallback.rs

@@ -15,6 +15,7 @@ mod set;
 use crate::ids::FallbackId;
 use derive_builder::Builder;
 use tor_config::ConfigBuildError;
+use tor_config::{define_list_builder_accessors, list_builder::VecBuilder};
 use tor_llcrypto::pk::ed25519::Ed25519Identity;
 use tor_llcrypto::pk::rsa::RsaIdentity;
 
@@ -33,7 +34,7 @@ pub use set::{FallbackList, FallbackListBuilder};
 // structure: we want our fallback directory configuration format to
 // be future-proof against adding new info about each fallback.
 #[derive(Debug, Clone, Deserialize, Builder, Eq, PartialEq)]
-#[builder(build_fn(validate = "FallbackDirBuilder::validate", error = "ConfigBuildError"))]
+#[builder(build_fn(private, name = "build_unvalidated", error = "ConfigBuildError"))]
 #[builder(derive(Deserialize))]
 pub struct FallbackDir {
     /// RSA identity for the directory relay
@@ -41,9 +42,16 @@ pub struct FallbackDir {
     /// Ed25519 identity for the directory relay
     ed_identity: Ed25519Identity,
     /// List of ORPorts for the directory relay
+    #[builder(sub_builder(fn_name = "build"), setter(custom))]
     orports: Vec<SocketAddr>,
 }
 
+define_list_builder_accessors! {
+    struct FallbackDirBuilder {
+        pub orports: [SocketAddr],
+    }
+}
+
 impl FallbackDir {
     /// Return a builder that can be used to make a `FallbackDir`.
     pub fn builder() -> FallbackDirBuilder {
@@ -67,24 +75,21 @@ impl FallbackDirBuilder {
     pub fn new() -> Self {
         Self::default()
     }
-    /// Add a single OR port for this fallback directory.
+    /// Builds a new `FallbackDir`.
     ///
-    /// This field is required, and may be called more than once.
-    pub fn orport(&mut self, orport: SocketAddr) -> &mut Self {
-        self.orports.get_or_insert_with(Vec::new).push(orport);
-        self
-    }
-    /// Check whether this builder is ready to make a FallbackDir.
-    fn validate(&self) -> std::result::Result<(), ConfigBuildError> {
-        if let Some(orports) = &self.orports {
-            if orports.is_empty() {
-                return Err(ConfigBuildError::Invalid {
-                    field: "orport".to_string(),
-                    problem: "list was empty".to_string(),
-                });
-            }
+    /// ### Errors
+    ///
+    /// Errors unless both of `rsa_identity`, `ed_identity`, and at least one `orport`,
+    /// have been provided.
+    pub fn build(&self) -> std::result::Result<FallbackDir, ConfigBuildError> {
+        let built = self.build_unvalidated()?;
+        if built.orports.is_empty() {
+            return Err(ConfigBuildError::Invalid {
+                field: "orport".to_string(),
+                problem: "list was empty".to_string(),
+            });
         }
-        Ok(())
+        Ok(built)
     }
 }
 
@@ -104,7 +109,7 @@ pub(crate) fn default_fallbacks() -> Vec<FallbackDirBuilder> {
             .iter()
             .map(|s| s.parse().expect("Bad socket address in fallbacklist"))
             .for_each(|p| {
-                bld.orport(p);
+                bld.orports().push(p);
             });
 
         bld

crates/tor-guardmgr/src/fallback/set.rs

@@ -244,12 +244,12 @@ mod test {
         let ed: [u8; 32] = rng.gen();
         let rsa: [u8; 20] = rng.gen();
         let ip: u32 = rng.gen();
-        FallbackDir::builder()
-            .ed_identity(ed.into())
+        let mut bld = FallbackDir::builder();
+        bld.ed_identity(ed.into())
             .rsa_identity(rsa.into())
-            .orport(std::net::SocketAddrV4::new(ip.into(), 9090).into())
-            .build()
-            .unwrap()
+            .orports()
+            .push(std::net::SocketAddrV4::new(ip.into(), 9090).into());
+        bld.build().unwrap()
     }
 
     #[test]

doc/semver_status.md

@@ -60,6 +60,7 @@ BREAKING: DirMgrCfg: schedule and network fields rename (`_config` removed)
 
 ### tor-guardmgr
 
+BREAKING: FallbackDirBuilder::orport() removed, and orports() now gives &mut SocketAddrListBuilder
 MODIFIED: New functions to get estimated clock skew.
 MODIFIED: New functions to report observed clock skew.
 BREAKING: Guard restriction builder interface changed to new list builder API.
@@ -84,4 +85,5 @@ BREAKING: Removed some unused accessors.
 
 ### tor-config
 
-MODIFIED: New macros define_list_config_builder macro_first_nonempty
+MODIFIED: New facilities for lists in builders (list_builder module, etc.)
+MODIFIED: New macro macro_first_nonempty