Commit a020f82d authored by Ian Jackson's avatar Ian Jackson
Browse files

DirMgrConfig: abolish builder; make it transparent and exhaustive 

See rationale in the comment.
parent ab352881

crates/arti-client/src/client.rs

+2 −2
Original line number Diff line number Diff line
@@ -332,7 +332,7 @@ impl<R: Runtime> TorClient<R> { 
        autobootstrap: BootstrapBehavior,

        dirmgr_builder: &dyn crate::builder::DirProviderBuilder<R>,

    ) -> StdResult<Self, ErrorDetail> {

        let dir_cfg = config.get_dirmgr_config()?;

        let dir_cfg = (&config).try_into()?;

        let statemgr = FsStateMgr::from_path(config.storage.expand_state_dir()?)?;

        let addr_cfg = config.address_filter.clone();
@@ -540,7 +540,7 @@ impl<R: Runtime> TorClient<R> { 
            _ => {}

        }



        let dir_cfg = new_config.get_dirmgr_config().map_err(wrap_err)?;

        let dir_cfg = new_config.try_into().map_err(wrap_err)?;

        let state_cfg = new_config.storage.expand_state_dir().map_err(wrap_err)?;

        let addr_cfg = &new_config.address_filter;

        let timeout_cfg = &new_config.stream_timeouts;

crates/arti-client/src/config.rs

+16 −13
Original line number Diff line number Diff line
@@ -15,6 +15,7 @@ use derive_builder::Builder; 
use derive_more::AsRef;

use serde::Deserialize;

use std::collections::HashMap;

use std::convert::TryInto;

use std::path::Path;

use std::path::PathBuf;

use std::time::Duration;
@@ -31,9 +32,9 @@ pub mod circ { 
/// Types for configuring how Tor accesses its directory information.

pub mod dir {

    pub use tor_dirmgr::{

        Authority, AuthorityBuilder, DirMgrConfig, DirMgrConfigBuilder, DownloadSchedule,

        DownloadScheduleConfig, DownloadScheduleConfigBuilder, FallbackDir, FallbackDirBuilder,

        NetworkConfig, NetworkConfigBuilder,

        Authority, AuthorityBuilder, DirMgrConfig, DownloadSchedule, DownloadScheduleConfig,

        DownloadScheduleConfigBuilder, FallbackDir, FallbackDirBuilder, NetworkConfig,

        NetworkConfigBuilder,

    };

}
@@ -323,17 +324,19 @@ impl TorClientConfig { 
    pub fn builder() -> TorClientConfigBuilder {

        TorClientConfigBuilder::default()

    }



    /// Build a DirMgrConfig from this configuration.

    pub(crate) fn get_dirmgr_config(&self) -> Result<dir::DirMgrConfig, ConfigBuildError> {

        let mut dircfg = dir::DirMgrConfigBuilder::default();

        dircfg.network_config(self.tor_network.clone());

        dircfg.schedule_config(self.download_schedule.clone());

        dircfg.cache_path(self.storage.expand_cache_dir()?);

        for (k, v) in self.override_net_params.iter() {

            dircfg.override_net_param(k.clone(), *v);

}

        dircfg.build()



impl TryInto<dir::DirMgrConfig> for &TorClientConfig {

    type Error = ConfigBuildError;



    #[rustfmt::skip]

    fn try_into(self) -> Result<dir::DirMgrConfig, ConfigBuildError> {

        Ok(dir::DirMgrConfig {

            network_config:      self.tor_network        .clone(),

            schedule_config:     self.download_schedule  .clone(),

            cache_path:          self.storage.expand_cache_dir()?,

            override_net_params: self.override_net_params.clone(),

        })

    }

}

crates/tor-dirmgr/src/config.rs

+32 −56
Original line number Diff line number Diff line
@@ -151,28 +151,36 @@ impl DownloadScheduleConfig { 


/// Configuration type for network directory operations.

///

/// This type is immutable once constructed.

/// If the directory manager gains new configurabilities, this structure will gain additional

/// supertraits, as an API break.

///

/// To create an object of this type, use [`DirMgrConfigBuilder`], or

/// deserialize it from a string. (Arti generally uses Toml for configuration,

/// but you can use other formats if you prefer.)

///

/// Many members of this type can be replaced with a new configuration on a

/// running Arti client. Those that cannot are documented.

#[derive(Debug, Clone, Builder, Eq, PartialEq)]

#[builder(build_fn(error = "ConfigBuildError"))]

#[builder(derive(Deserialize))]

/// Prefer to use `TorClientConfig`, which will always be convertible to this struct

/// via `TryInto`.

//

// We do not use a builder here.  Instead, additions or changes here are API breaks.

//

// Rationale:

//

// The purpose of using a builder is to allow the code to continue to

// compile when new fields are added to the built struct.

//

// However, here, the DirMgrConfig is just a subset of the fields of a

// TorClientConfig, and it is important that all its fields are

// initialised by arti-client.

//

// If it grows a field, arti-client ought not to compile any more.

#[derive(Debug, Clone)]

#[cfg_attr(test, derive(Default))]

#[allow(clippy::exhaustive_structs)]

pub struct DirMgrConfig {

    /// Location to use for storing and reading current-format

    /// directory information.

    ///

    /// Cannot be changed on a running Arti client.

    #[builder(setter(into))]

    cache_path: PathBuf,

    pub cache_path: PathBuf,



    /// Configuration information about the network.

    #[builder(default)]

    network_config: NetworkConfig,

    pub network_config: NetworkConfig,



    /// Configuration information about when we download things.

    ///
@@ -184,8 +192,7 @@ pub struct DirMgrConfig { 
    /// on in-progress attempts as well, at least at the top level.  Users

    /// should _not_ assume that the effect of changing this option will always

    /// be delayed.)

    #[builder(default)]

    schedule_config: DownloadScheduleConfig,

    pub schedule_config: DownloadScheduleConfig,



    /// A map of network parameters that we're overriding from their settings in

    /// the consensus.
@@ -196,36 +203,10 @@ pub struct DirMgrConfig { 
    /// (The above is a limitation: we would like it to someday take effect

    /// immediately. Users should _not_ assume that the effect of changing this

    /// option will always be delayed.)

    #[builder(default)]

    override_net_params: netstatus::NetParams<i32>,

}



impl DirMgrConfigBuilder {

    /// Overrides the network consensus parameter named `param` with a

    /// new value.

    ///

    /// If the new value is out of range, it will be clamped to the

    /// acceptable range.

    ///

    /// If the parameter is not recognized by Arti, it will be

    /// ignored, and a warning will be produced when we try to apply

    /// it to the consensus.

    ///

    /// By default no parameters will be overridden.

    pub fn override_net_param(&mut self, param: String, value: i32) -> &mut Self {

        self.override_net_params

            .get_or_insert_with(netstatus::NetParams::default)

            .set(param, value);

        self

    }

    pub override_net_params: netstatus::NetParams<i32>,

}



impl DirMgrConfig {

    /// Return a new builder to construct a DirMgrConfig.

    pub fn builder() -> DirMgrConfigBuilder {

        DirMgrConfigBuilder::default()

    }



    /// Create a store from this configuration.

    ///

    /// Note that each time this is called, a new store object will be
@@ -354,10 +335,10 @@ mod test { 
    fn simplest_config() -> Result<()> {

        let tmp = tempdir().unwrap();



        let dir = DirMgrConfigBuilder::default()

            .cache_path(tmp.path().to_path_buf())

            .build()

            .unwrap();

        let dir = DirMgrConfig {

            cache_path: tmp.path().into(),

            ..Default::default()

        };



        assert!(dir.authorities().len() >= 3);

        assert!(dir.fallbacks().len() >= 3);
@@ -434,18 +415,13 @@ mod test { 


    #[test]

    fn build_dirmgrcfg() -> Result<()> {

        let mut bld = DirMgrConfig::builder();

        let mut bld = DirMgrConfig::default();

        let tmp = tempdir().unwrap();



        let cfg = bld

            .override_net_param("circwindow".into(), 999)

            .cache_path(tmp.path())

            .network_config(NetworkConfig::default())

            .schedule_config(DownloadScheduleConfig::default())

            .build()

            .unwrap();

        bld.override_net_params.set("circwindow".into(), 999);

        bld.cache_path = tmp.path().into();



        assert_eq!(cfg.override_net_params().get("circwindow").unwrap(), &999);

        assert_eq!(bld.override_net_params().get("circwindow").unwrap(), &999);



        Ok(())

    }

crates/tor-dirmgr/src/lib.rs

+6 −6
Original line number Diff line number Diff line
@@ -90,8 +90,8 @@ use std::{fmt::Debug, time::SystemTime}; 


pub use authority::{Authority, AuthorityBuilder};

pub use config::{

    DirMgrConfig, DirMgrConfigBuilder, DownloadScheduleConfig, DownloadScheduleConfigBuilder,

    NetworkConfig, NetworkConfigBuilder,

    DirMgrConfig, DownloadScheduleConfig, DownloadScheduleConfigBuilder, NetworkConfig,

    NetworkConfigBuilder,

};

pub use docid::DocId;

pub use err::Error;
@@ -1026,10 +1026,10 @@ mod test { 


    pub(crate) fn new_mgr<R: Runtime>(runtime: R) -> (TempDir, DirMgr<R>) {

        let dir = TempDir::new().unwrap();

        let config = DirMgrConfig::builder()

            .cache_path(dir.path())

            .build()

            .unwrap();

        let config = DirMgrConfig {

            cache_path: dir.path().into(),

            ..Default::default()

        };

        let dirmgr = DirMgr::from_config(config, runtime, None, false).unwrap();



        (dir, dirmgr)

crates/tor-dirmgr/src/state.rs

+5 −5
Original line number Diff line number Diff line
@@ -966,11 +966,11 @@ mod test { 
            if let Some(a) = authorities {

                netcfg.authorities(a);

            }

            let cfg = DirMgrConfig::builder()

                .cache_path("/we_will_never_use_this/")

                .network_config(netcfg.build().unwrap())

                .build()

                .unwrap();

            let cfg = DirMgrConfig {

                cache_path: "/we_will_never_use_this/".into(),

                network_config: netcfg.build().unwrap(),

                ..Default::default()

            };

            let cfg = Arc::new(cfg);

            DirRcv {

                now,