Ticket32487 #1644

nmathewson · 2020-01-09T03:00:49Z

No description provided.

For now, this module is enabled whenever the relay module is enabled, and disabled whenever the relay module is disabled. Though they are logically separate, the use cases for running one without the other are rare enough that we don't really want to support compiling them independently.

Only directory caches actually need to spool things.

This function had some XXX comments indicating (correctly) that it was not actually used by the dirserver code, and that only the controller still used it.

This is cleaner than iterating over the spool.

To make Tor still work, we define a minimal dircache_stub.c file that defines the entry points to the module that can actually be seen by the compiler when we're building with dircache and relay disabled.

coveralls · 2020-01-09T03:21:37Z

Pull Request Test Coverage Report for Build 7788

50 of 95 (52.63%) changed or added relevant lines in 12 files are covered.
1351 unchanged lines in 18 files lost coverage.
Overall coverage increased (+0.1%) to 63.319%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/feature/nodelist/nodelist.c	0	1	0.0%
src/core/mainloop/mainloop.c	1	3	33.33%
src/feature/dirclient/dirclient.c	2	6	33.33%
src/feature/control/control_getinfo.c	0	38	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/core/mainloop/mainloop.c	1	37.55%
src/core/or/channeltls.c	1	42.54%
src/feature/hs/hs_service.c	1	73.29%
src/feature/hs/hs_circuit.c	2	41.57%
src/feature/nodelist/nodelist.c	2	71.66%
src/core/mainloop/connection.h	3	50.0%
src/feature/dirauth/shared_random.c	4	85.0%
src/feature/dirauth/authmode.c	5	66.67%
src/feature/dirauth/dirvote.c	9	64.85%
src/app/main/main.c	10	22.02%

Totals
Change from base Build 7714:	0.1%
Covered Lines:	49949
Relevant Lines:	78885

💛 - Coveralls

teor2345 · 2020-01-15T03:31:19Z

doc/HACKING/Module.md

+being progressively refactored and disabled.
+
+The dircache code is located in `src/*/*dircache`.  Right now, it is
+enabled if and only if the relay module is disabled.  (We are treating


Suggested change

enabled if and only if the relay module is disabled. (We are treating

disabled if and only if the relay module is disabled. (We are treating

Fixed in 44dd607

teor2345 · 2020-01-15T03:33:07Z

src/core/mainloop/fooz

@@ -0,0 +1,10 @@
+


This file appears to be a temporary file, that was committed by mistake.

Removed in 8df602e

teor2345 · 2020-01-15T03:39:45Z

src/feature/dircache/dircache_stub.c

+  return -1;
+}
+
+int


We usually stub out functions like this using macros, because it lets the optimiser eliminate more dead code. This comment applies to all the stub functions that return a value, particularly if they have no side effects.

(The functions with side effects could also become inline functions in the header.)

For example:
https://github.com/torproject/tor/blob/master/src/feature/relay/routermode.h#L28

teor2345 · 2020-01-15T03:46:15Z

src/feature/dircache/dircache_stub.c

+}
+
+int
+directory_fetches_dir_info_early(const or_options_t *options)


FetchDirInfoEarly is listed as a general option in the man page. It does work on clients, and it's used by some custom client setups (like bandwidth scanners, and other directory monitors).

I think it's ok to disable this option when dircache mode is disabled, but we need to document that in the man page. Maybe it should be moved to an "Advanced Options" section, with a note that these options don't work on clients, when compiled with dircache mode disabled?

(This function calls directory_fetches_from_authorities(), which checks the value of FetchDirInfoEarly.)

These comments also apply to FetchDirInfoExtraEarly.

There are a bunch of related options used by should_use_directory_guards():

UseEntryGuards

DownloadExtraInfo

FetchDirInfoEarly

FetchDirInfoExtraEarly

FetchUselessDescriptors

tor/src/feature/dirclient/dirclient.c

Line 310 in 4f02812

/** Return true iff, according to the values in <b>options</b>, we should be

For consistency, we should probably either keep all these options, or disable all of them. If we decide to disable them, let's do it in a separate ticket, and make directory_fetches_dir_info_early() a dircommon function for now?

teor2345 · 2020-01-15T03:47:55Z

src/feature/dircache/dircache_stub.c

+  return 0;
+}
+
+int


Despite its name, this is actually a dirclient function, which depends on the value of UseBridges.

teor2345 · 2020-01-15T03:54:17Z

src/feature/dircache/dircache_stub.c

+}
+
+int
+directory_too_idle_to_fetch_descriptors(const or_options_t *options,


Despite its name, this is actually a dirclient function:

it depends on rep_hist_circbuilding_dormant(), which is a client function, and

it depends on FetchUselessDescriptors, which is a general option in the man page, but probably belongs in an "Advanced Options" section, like FetchDirInfoEarly.

(Unlike FetchDirInfoEarly, disabling the dircache module doesn't actually disable FetchUselessDescriptors, but I wouldn't mind if it did.)

But if we decide to disable FetchUselessDescriptors when dircache is disabled, let's do it in a separate ticket.

teor2345 · 2020-01-15T04:05:15Z

configure.ac

@@ -289,6 +289,14 @@ AM_COND_IF(BUILD_MODULE_RELAY,
           AC_DEFINE([HAVE_MODULE_RELAY], [1],
                     [Compile with Relay feature support]))

+dnl Dircache module.  (This cannot be enabled or disabled independently of


Please add a note that this module isn't listed in --list-modules.

We also don't have a have_module_dircache() function. I don't mind either way, but we should either have the function, or have a comment in dircache.h explaining why it doesn't exist.

We don't check for dircache in list_enabled_modules(). That's actually a good thing, because if we did, we'd have to make a large number of changes to the test cases in test_parseconf.sh. Let's add comments in list_enabled_modules() and test_parseconf.sh explaining why the dircache module isn't listed.

If you do want to list the dircache module, please make test_parseconf.sh ignore the dircache module, so we don't have to modify a whole bunch of tests. (By adding dircache_ to the relay_ disabled module file names.)

Remove an accidentally committed temporary file.

s/enabled/disabled/

I had incorrectly identified these functions as dircache-only, when in fact they apply to everyone who acts a directory client.

This is an automated commit, generated by this command: ./scripts/maint/rename_c_identifier.py \ directory_must_use_begindir dirclient_must_use_begindir \ directory_fetches_from_authorities dirclient_fetches_from_authorities \ directory_fetches_dir_info_early dirclient_fetches_dir_info_early \ directory_fetches_dir_info_later dirclient_fetches_dir_info_later \ directory_too_idle_to_fetch_descriptors dirclient_too_idle_to_fetch_descriptors

This may help the compiler eliminate deadcode.

teor2345 · 2020-01-16T02:00:38Z

src/app/config/config.c

@@ -2733,6 +2733,9 @@ list_enabled_modules(void)
 {
  printf("%s: %s\n", "relay", have_module_relay() ? "yes" : "no");
  printf("%s: %s\n", "dirauth", have_module_dirauth() ? "yes" : "no");
+  // We don't list dircache, because it cannot be enabled or disabled
+  // independently from relay.  Listing it here would proliferate
+  // tests in test_parseconf.c to no useful purpose.


It's actually the parseconf script, not the unit tests :-)

Suggested change

// tests in test_parseconf.c to no useful purpose.

// test variants in test_parseconf.sh to no useful purpose.

refer to the correct tests

Fix compilation errors.

nmathewson added 6 commits Jan 9, 2020

Move dir_split_resource_into_spoolable() to dircache module.

28f4ac4

Only directory caches actually need to spool things.

Move dirserv_get_routerdescs() to control_getinfo.c

93fa7c0

This function had some XXX comments indicating (correctly) that it was not actually used by the dirserver code, and that only the controller still used it.

Use dir_conn_clear_spool() in connection.c.

80e739b

This is cleaner than iterating over the spool.

Disable feature/dircache files when dircache module is disabled.

ab810e0

To make Tor still work, we define a minimal dircache_stub.c file that defines the entry points to the module that can actually be seen by the compiler when we're building with dircache and relay disabled.

Add a changes file for disabling the dircache code.

130251e

teor2345 reviewed Jan 15, 2020

View changes

src/core/mainloop/fooz Outdated

@@ -0,0 +1,10 @@

Copy link

Contributor

teor2345 Jan 15, 2020 •

edited

This file appears to be a temporary file, that was committed by mistake.

Copy link

Contributor Author

nmathewson Jan 15, 2020

Removed in 8df602e

teor2345 reviewed Jan 15, 2020

View changes

src/feature/dircache/dircache_stub.c Outdated

return 0;

}

int

Copy link

Contributor

teor2345 Jan 15, 2020 •

edited

Despite its name, this is actually a dirclient function, which depends on the value of UseBridges.

teor2345 reviewed Jan 15, 2020

View changes

nmathewson added 8 commits Jan 15, 2020

fixup! Use dir_conn_clear_spool() in connection.c.

8df602e

Remove an accidentally committed temporary file.

fixup! Define a "dircache" module.

44dd607

s/enabled/disabled/

Move dirclient-related functions out of dirserv, and reenable them

4ede820

I had incorrectly identified these functions as dircache-only, when in fact they apply to everyone who acts a directory client.

Move directory_must_use_begindir() to dirclient_modes.[ch]

bf75e8c

Turn several functions from stubs into macros

f0314a1

This may help the compiler eliminate deadcode.

Add have_module_dircache().

dfe3287

Document why dircache is not included in --list-modules

bfac319

teor2345 reviewed Jan 16, 2020

View changes

nmathewson added 2 commits Jan 16, 2020

fixup! Document why dircache is not included in --list-modules

01aabf3

refer to the correct tests

fixup! Turn several functions from stubs into macros

ddb23f5

Fix compilation errors.

torproject-pusher closed this May 25, 2021

torproject-pusher deleted the branch torproject:master May 25, 2021

torproject / tor Public

Ticket32487 #1644

Ticket32487 #1644

nmathewson commented Jan 9, 2020

coveralls commented Jan 9, 2020 •

edited

teor2345 Jan 15, 2020

nmathewson Jan 15, 2020

teor2345 Jan 15, 2020 •

edited

nmathewson Jan 15, 2020

teor2345 Jan 15, 2020

teor2345 Jan 15, 2020

teor2345 Jan 15, 2020

teor2345 Jan 15, 2020

teor2345 Jan 15, 2020

teor2345 Jan 15, 2020 •

edited

teor2345 Jan 15, 2020 •

edited

teor2345 Jan 15, 2020

teor2345 Jan 15, 2020

teor2345 Jan 15, 2020

teor2345 Jan 15, 2020

teor2345 Jan 16, 2020

	enabled if and only if the relay module is disabled. (We are treating
	disabled if and only if the relay module is disabled. (We are treating

	// tests in test_parseconf.c to no useful purpose.
	// test variants in test_parseconf.sh to no useful purpose.

torproject / tor Public

Ticket32487 #1644

Ticket32487 #1644

Conversation

nmathewson commented Jan 9, 2020

coveralls commented Jan 9, 2020 • edited

Pull Request Test Coverage Report for Build 7788

💛 - Coveralls

teor2345 Jan 15, 2020 • edited

teor2345 Jan 15, 2020 • edited

teor2345 Jan 15, 2020 • edited

coveralls commented Jan 9, 2020 •

edited

teor2345 Jan 15, 2020 •

edited

teor2345 Jan 15, 2020 •

edited

teor2345 Jan 15, 2020 •

edited