Re: [proposal 136] Re: Proposal: Simplify Configuration of Private Tor Networks

[Nick Mathewson <nickm@xxxxxxxxxxxxx>]

On Wed, May 28, 2008 at 02:15:49PM +0200, Karsten Loesing wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Roger Dingledine wrote:
> | The var->initvalue = tor_strdup(val) above clobbers the current value of
> | var->initvalue. For the first time we assign config options, that's fine,
> | since its initial value is from a static table. But for future times we
> | assign config options, we'll leak the previous value.
> |
> | One better approach might be to have a static table of keys (strings)
> | and values (strings) for the alternate defaults, and walk through the
> | table doing a config_find_option() on the key and then assigning value
> | directly from the table (rather than making a copy).
> 
> You are right. I fixed that as you proposed (or similarly) and attached
> a new patch.

Hi, Karsten!  This looks pretty good to me.  There are a few
issues remaining; let me know if you'd like me to fix them
myself, or if you'd like to do it.

The biggest issue is that there's no documentation for these new
options in the manpage.  They probably want a new section of their
own, since they're only useful for testing.

> +  if (options->V3AuthInitialVotingInterval != 30*60 &&
> +      !options->TestingTorNetwork) {

Hmmm.  I don't like encoding the default explicitly in two places.
It's not obvious above that if somebody changes the default from "30
minutes" they also need to change the test here.  The long-term fix is
probably a more general config_is_default() function, but for now I
don't see a trivial fix.

> +    REJECT("V3AuthInitialVotingInterval may only be changed in testing "
> +           "Tor networks!");
> +  } else if (options->V3AuthInitialVotingInterval < MIN_VOTE_INTERVAL) {
> +    REJECT("V3AuthInitialVotingInterval is insanely low.");
> +  } else if (options->V3AuthInitialVotingInterval > 24*60*60) {
> +    REJECT("V3AuthInitialVotingInterval is insanely high.");

This test is redundant with the test below; there isn't much reason to
say "too high" for everything over a day, and "doesn't divide 30
minutes evenly" for everything between 31 minutes and a day.

> +  } else if (((30*60) % options->V3AuthInitialVotingInterval) != 0) {
> +    REJECT("V3AuthInitialVotingInterval does not divide evenly into "
> +           "30 minutes.");
> +  }
 [...]
> +  /* If this is a testing network configuration, change defaults
> +   * for a list of dependent config options, re-initialize newoptions
> +   * with the new defaults, and assign all options to it second time. */
> +  if (newoptions->TestingTorNetwork) {
> +
> +    /* Change defaults. */
> +    int i;
> +    for (i = 0; testing_tor_network_defaults[i].name; ++i) {
> +      config_var_t *new_var = &testing_tor_network_defaults[i];
> +      config_var_t *old_var =
> +          config_find_option(&options_format, new_var->name);
> +      tor_assert(new_var);
> +      tor_assert(old_var);
> +      old_var->initvalue = new_var->initvalue;
> +    }
> +
> +    /* Clear newoptions and re-initialize them with new defaults. */
> +    config_free(&options_format, newoptions);
> +    newoptions = tor_malloc_zero(sizeof(or_options_t));
> +    newoptions->_magic = OR_OPTIONS_MAGIC;
> +    options_init(newoptions);
> +    newoptions->command = command;
> +    newoptions->command_arg = command_arg;
> +
> +    /* Assign all options a second time. */
> +    retval = config_get_lines(cf, &cl);
> +    if (retval < 0) {
> +      err = SETOPT_ERR_PARSE;
> +      goto err;
> +    }

Wow.  This whole "parse it twice" business is a little hackish, but I
don't see a better way to do it offhand, so I'll not argue.