Skip to content

NetParams: Document its role as a validated config parameter

Ian Jackson requested to merge Diziet/arti:netparams into main

In #413 (closed) and a code comment, I grumbled about NetParams. Having investigated, my conclusions are as follows.

Facts

  • NetParams<T> has as its primary use in elements of the state of the dirmgr, tor-netdoc, etc. There are at least two specialisations and various places where this data appears.
  • tor-dirmgr takes a config setting for allowing the config to override some network parameters - network parameters that it handles internally as a NetParams<i32>. For convenience of the code there, it just reuses the NetParams<i32> as the built (validated) config element. DirMgrConfig is not a builder struct (for reasons explained in its comments); instead, it is simply a copy of some of the pieces of a TorClientConfig.
  • Therefore, a validated TorClientConfig contains override_net_params: NetParams<i32>.
  • derive_builder attrs are used to hide all this in the TorClientConfigBuilder. The builder (the public API) contains a plain HashMap and offers an override_net_params() mut accessor function.

So the NetParams is already not exposed in the primary config API - only in lower levels. The primary config API is precisely to allow the user to build and manipulate a HashMap<String,i32>. Likewise, we deserialise it as a HashMap defaulting to empty.

Offering a concrete struct in this way is roughly in line with what we now do for lists, where we offer accessors for a Vec. However, there is a difference: the Vec arrangements are capable of having a non-empty default. Here, the semantics of the setting imply that it has an empty default.

Choices for the top-level API

Overall I think the choices for the top-level config API here are:

  1. As things are now. Expose a HashMap directly, with no _opt etc. (ie no non-empty defaulting). If we want to add config entries in the future which are of this rough shape but with a non-empty default, they might need a more sophisticated API. However, the current API mirrors the one for Vecs pretty precisely, so I think that future change would be an extension even if we were to retrofit it to override_net_params.

  2. Build, now, a thing for HashMap that mirrors the list builder for Vec. This seems unattractive at this stage, particularly since I think that API would be a superset of what we have now.

  3. Make the builder have a Vec and offer the list API, even though this is actually a non-ordered set of overrides (or, rather, a set of overrides which honours ordering only in that it forgets earlier settings). This seems quite unattractive, as it would be a misrepresentation of the semantics. And it would either have to be Vec<(String, Option<i32>)> so that it can support deletion by appending, or deletion would have to be done by filtering even though overriding can be done by appending.

  4. As we have now, but hide the HashMap behind something with a less rich API. There is an argument for this - our reasoning for Vec was that Vec is a very fundamental type in Rust, and this is less so for HashMap. Nevertheless I don't foresee us wanting to change this.

Of these I prefer (1), the status quo. (2) and (3) are quite unattractive.

NetParams as both state and validated config

Finally, it is sligbhtly anomalous that NetParams<i32> is both a piece of state and a piece of config. But this is in the lower layers and I don't see any value in separating these types - it would just lead to pointless conversion code. I think comments are fine to address this.

We could add type aliases (eg type NetParamsBuilder<T> = HashMap<String,T>, but IMO they would confuse and indirect more than they would enlighten.

Disposition

Therefore I proose here to address #413 (closed) in code comments, and make no other code changes.

Would the reviewer please double check my reasoning above :-). If I have got anything wrong, or you disagree with this resolution, we should talk about it and not merge this MR.

Merge request reports