From c9729853a5ccfc15d206c43be8e15aea93ae19ee Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Mon, 2 Oct 2017 20:43:22 +0300 Subject: [PATCH 1/5] entrynodes: Be specific about how many primary descriptors we miss. --- src/or/entrynodes.c | 29 ++++++++++++++++++++--------- src/or/entrynodes.h | 5 ++--- src/or/nodelist.c | 13 ++++++++----- src/test/test_entrynodes.c | 19 +++++++++++++++++++ 4 files changed, 49 insertions(+), 17 deletions(-) diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index 129b983334..1e96fc7f54 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -3440,15 +3440,16 @@ guards_retry_optimistic(const or_options_t *options) } /** - * Return true iff we know enough directory information to construct - * circuits through all of the primary guards we'd currently use. - */ -int -guard_selection_have_enough_dir_info_to_build_circuits(guard_selection_t *gs) + * Check if we are missing any crucial dirinfo for the guard subsystem to + * work. Return NULL if everything went well, otherwise return a newly + * allocated string with an informative error message. */ +char * +guard_selection_get_dir_info_status_str(guard_selection_t *gs) { if (!gs->primary_guards_up_to_date) entry_guards_update_primary(gs); + char *ret_str = NULL; int n_missing_descriptors = 0; int n_considered = 0; int num_primary_to_check; @@ -3470,15 +3471,25 @@ guard_selection_have_enough_dir_info_to_build_circuits(guard_selection_t *gs) break; } SMARTLIST_FOREACH_END(guard); - return n_missing_descriptors == 0; + /* If we are not missing any descriptors, return NULL. */ + if (!n_missing_descriptors) { + return NULL; + } + + /* otherwise return a helpful error string */ + tor_asprintf(&ret_str, "We're missing descriptors for %d/%d of our " + "primary entry guards", + n_missing_descriptors, num_primary_to_check); + + return ret_str; } /** As guard_selection_have_enough_dir_info_to_build_circuits, but uses * the default guard selection. */ -int -entry_guards_have_enough_dir_info_to_build_circuits(void) +char * +entry_guards_get_dir_info_status_str(void) { - return guard_selection_have_enough_dir_info_to_build_circuits( + return guard_selection_get_dir_info_status_str( get_guard_selection_info()); } diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index f74ccd97f4..d049a88cda 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -572,9 +572,8 @@ int getinfo_helper_entry_guards(control_connection_t *conn, int entries_known_but_down(const or_options_t *options); void entries_retry_all(const or_options_t *options); -int guard_selection_have_enough_dir_info_to_build_circuits( - guard_selection_t *gs); -int entry_guards_have_enough_dir_info_to_build_circuits(void); +char *entry_guards_get_dir_info_status_str(void); +char *guard_selection_get_dir_info_status_str(guard_selection_t *gs); void entry_guards_free_all(void); diff --git a/src/or/nodelist.c b/src/or/nodelist.c index df735a9d24..93dd43f3cd 100644 --- a/src/or/nodelist.c +++ b/src/or/nodelist.c @@ -2300,11 +2300,14 @@ update_router_have_minimum_dir_info(void) using_md = consensus->flavor == FLAV_MICRODESC; - if (! entry_guards_have_enough_dir_info_to_build_circuits()) { - strlcpy(dir_info_status, "We're missing descriptors for some of our " - "primary entry guards", sizeof(dir_info_status)); - res = 0; - goto done; + { /* Check entry guard dirinfo status */ + char *guard_error = entry_guards_get_dir_info_status_str(); + if (guard_error) { + strlcpy(dir_info_status, guard_error, sizeof(dir_info_status)); + tor_free(guard_error); + res = 0; + goto done; + } } /* Check fraction of available paths */ diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c index 1e008c3a2f..36e457bdf1 100644 --- a/src/test/test_entrynodes.c +++ b/src/test/test_entrynodes.c @@ -1639,6 +1639,25 @@ test_entry_guard_manage_primary(void *arg) tt_ptr_op(g, OP_EQ, smartlist_get(prev_guards, g_sl_idx)); }); + /* Do some dirinfo checks */ + { + /* Check that we have all required dirinfo for the primaries (that's done in + * big_fake_network_setup()) */ + char *dir_info_str = guard_selection_get_dir_info_status_str(gs); + tt_assert(!dir_info_str); + + /* Now artificially remove the first primary's descriptor and re-check */ + entry_guard_t *first_primary; + first_primary = smartlist_get(gs->primary_entry_guards, 0); + /* Change the first primary's identity digest so that the mocked functions + * can't find its descriptor */ + memset(first_primary->identity, 9, sizeof(first_primary->identity)); + dir_info_str = guard_selection_get_dir_info_status_str(gs); + tt_str_op(dir_info_str, OP_EQ, + "We're missing descriptors for 1/2 of our primary entry guards"); + tor_free(dir_info_str); + } + done: guard_selection_free(gs); smartlist_free(prev_guards); From f2231306ba58cc5d9a5addb41eb7f287e3802746 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Tue, 3 Oct 2017 14:16:49 +0300 Subject: [PATCH 2/5] entrynodes: Move guard dirinfo check below path dirinfo check. We do that because we want to use the path fraction dirinfo data in case we are missing primary guard dirinfo. --- src/or/nodelist.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/or/nodelist.c b/src/or/nodelist.c index 93dd43f3cd..ff26065ff2 100644 --- a/src/or/nodelist.c +++ b/src/or/nodelist.c @@ -2300,16 +2300,6 @@ update_router_have_minimum_dir_info(void) using_md = consensus->flavor == FLAV_MICRODESC; - { /* Check entry guard dirinfo status */ - char *guard_error = entry_guards_get_dir_info_status_str(); - if (guard_error) { - strlcpy(dir_info_status, guard_error, sizeof(dir_info_status)); - tor_free(guard_error); - res = 0; - goto done; - } - } - /* Check fraction of available paths */ { char *status = NULL; @@ -2334,6 +2324,17 @@ update_router_have_minimum_dir_info(void) res = 1; } + { /* Check entry guard dirinfo status */ + char *guard_error = entry_guards_get_dir_info_status_str(); + if (guard_error) { + strlcpy(dir_info_status, guard_error, sizeof(dir_info_status)); + tor_free(guard_error); + res = 0; + goto done; + } + } + + done: /* If paths have just become available in this update. */ From 5352785d0caec23f753ae7fcab4449b03f5643b8 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Tue, 3 Oct 2017 14:38:53 +0300 Subject: [PATCH 3/5] entrynodes: Error msg for missing guard descs is now more informative. --- src/or/entrynodes.c | 21 ++++++++++++++------- src/or/entrynodes.h | 7 +++++-- src/or/nodelist.c | 7 ++++--- src/test/test_entrynodes.c | 11 ++++++----- 4 files changed, 29 insertions(+), 17 deletions(-) diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index 1e96fc7f54..c71a93beae 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -3442,9 +3442,13 @@ guards_retry_optimistic(const or_options_t *options) /** * Check if we are missing any crucial dirinfo for the guard subsystem to * work. Return NULL if everything went well, otherwise return a newly - * allocated string with an informative error message. */ + * allocated string with an informative error message. In the latter case, use + * the genreal descriptor information using_mds, num_present and + * num_usable to improve the error message. */ char * -guard_selection_get_dir_info_status_str(guard_selection_t *gs) +guard_selection_get_dir_info_status_str(guard_selection_t *gs, + int using_mds, + int num_present, int num_usable) { if (!gs->primary_guards_up_to_date) entry_guards_update_primary(gs); @@ -3478,8 +3482,9 @@ guard_selection_get_dir_info_status_str(guard_selection_t *gs) /* otherwise return a helpful error string */ tor_asprintf(&ret_str, "We're missing descriptors for %d/%d of our " - "primary entry guards", - n_missing_descriptors, num_primary_to_check); + "primary entry guards (total %sdescriptors: %d/%d).", + n_missing_descriptors, num_primary_to_check, + using_mds?"micro":"", num_present, num_usable); return ret_str; } @@ -3487,10 +3492,12 @@ guard_selection_get_dir_info_status_str(guard_selection_t *gs) /** As guard_selection_have_enough_dir_info_to_build_circuits, but uses * the default guard selection. */ char * -entry_guards_get_dir_info_status_str(void) +entry_guards_get_dir_info_status_str(int using_mds, + int num_present, int num_usable) { - return guard_selection_get_dir_info_status_str( - get_guard_selection_info()); + return guard_selection_get_dir_info_status_str(get_guard_selection_info(), + using_mds, + num_present, num_usable); } /** Free one guard selection context */ diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index d049a88cda..ca9fb489e6 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -572,8 +572,11 @@ int getinfo_helper_entry_guards(control_connection_t *conn, int entries_known_but_down(const or_options_t *options); void entries_retry_all(const or_options_t *options); -char *entry_guards_get_dir_info_status_str(void); -char *guard_selection_get_dir_info_status_str(guard_selection_t *gs); +char *entry_guards_get_dir_info_status_str(int using_mds, + int num_present, int num_usable); +char *guard_selection_get_dir_info_status_str(guard_selection_t *gs, + int using_mds, + int num_present, int num_usable); void entry_guards_free_all(void); diff --git a/src/or/nodelist.c b/src/or/nodelist.c index ff26065ff2..6ca9bd6d4b 100644 --- a/src/or/nodelist.c +++ b/src/or/nodelist.c @@ -2282,6 +2282,7 @@ update_router_have_minimum_dir_info(void) { time_t now = time(NULL); int res; + int num_present=0, num_usable=0; const or_options_t *options = get_options(); const networkstatus_t *consensus = networkstatus_get_reasonably_live_consensus(now,usable_consensus_flavor()); @@ -2303,7 +2304,6 @@ update_router_have_minimum_dir_info(void) /* Check fraction of available paths */ { char *status = NULL; - int num_present=0, num_usable=0; double paths = compute_frac_paths_available(consensus, options, now, &num_present, &num_usable, &status); @@ -2325,7 +2325,9 @@ update_router_have_minimum_dir_info(void) } { /* Check entry guard dirinfo status */ - char *guard_error = entry_guards_get_dir_info_status_str(); + char *guard_error = entry_guards_get_dir_info_status_str(using_md, + num_present, + num_usable); if (guard_error) { strlcpy(dir_info_status, guard_error, sizeof(dir_info_status)); tor_free(guard_error); @@ -2334,7 +2336,6 @@ update_router_have_minimum_dir_info(void) } } - done: /* If paths have just become available in this update. */ diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c index 36e457bdf1..472383f382 100644 --- a/src/test/test_entrynodes.c +++ b/src/test/test_entrynodes.c @@ -1641,9 +1641,9 @@ test_entry_guard_manage_primary(void *arg) /* Do some dirinfo checks */ { - /* Check that we have all required dirinfo for the primaries (that's done in - * big_fake_network_setup()) */ - char *dir_info_str = guard_selection_get_dir_info_status_str(gs); + /* Check that we have all required dirinfo for the primaries (that's done + * in big_fake_network_setup()) */ + char *dir_info_str = guard_selection_get_dir_info_status_str(gs, 0, 0, 0); tt_assert(!dir_info_str); /* Now artificially remove the first primary's descriptor and re-check */ @@ -1652,9 +1652,10 @@ test_entry_guard_manage_primary(void *arg) /* Change the first primary's identity digest so that the mocked functions * can't find its descriptor */ memset(first_primary->identity, 9, sizeof(first_primary->identity)); - dir_info_str = guard_selection_get_dir_info_status_str(gs); + dir_info_str = guard_selection_get_dir_info_status_str(gs, 1, 2, 3); tt_str_op(dir_info_str, OP_EQ, - "We're missing descriptors for 1/2 of our primary entry guards"); + "We're missing descriptors for 1/2 of our primary entry guards " + "(total microdescriptors: 2/3)."); tor_free(dir_info_str); } From d891faddc79c8cb58723edd431895927871d3803 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Tue, 3 Oct 2017 15:48:12 +0300 Subject: [PATCH 4/5] entrynodes: Add changes file. --- changes/bug23670 | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changes/bug23670 diff --git a/changes/bug23670 b/changes/bug23670 new file mode 100644 index 0000000000..039bc39478 --- /dev/null +++ b/changes/bug23670 @@ -0,0 +1,3 @@ + o Minor features (entry guards): + - Improve logs issued when we are missing descriptors of primary guards. + Resolves ticket 23670. From f7306b16ecaeb07eb64d0fcbfcc43754533818c2 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Tue, 3 Oct 2017 15:50:54 +0300 Subject: [PATCH 5/5] entrynodes: Better naming for dir info check functions. --- src/or/entrynodes.c | 7 ++++--- src/or/entrynodes.h | 4 ++-- src/or/nodelist.c | 2 +- src/test/test_entrynodes.c | 5 +++-- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index c71a93beae..9fbf426433 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -3446,7 +3446,7 @@ guards_retry_optimistic(const or_options_t *options) * the genreal descriptor information using_mds, num_present and * num_usable to improve the error message. */ char * -guard_selection_get_dir_info_status_str(guard_selection_t *gs, +guard_selection_get_err_str_if_dir_info_missing(guard_selection_t *gs, int using_mds, int num_present, int num_usable) { @@ -3492,10 +3492,11 @@ guard_selection_get_dir_info_status_str(guard_selection_t *gs, /** As guard_selection_have_enough_dir_info_to_build_circuits, but uses * the default guard selection. */ char * -entry_guards_get_dir_info_status_str(int using_mds, +entry_guards_get_err_str_if_dir_info_missing(int using_mds, int num_present, int num_usable) { - return guard_selection_get_dir_info_status_str(get_guard_selection_info(), + return guard_selection_get_err_str_if_dir_info_missing( + get_guard_selection_info(), using_mds, num_present, num_usable); } diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index ca9fb489e6..9e1e729930 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -572,9 +572,9 @@ int getinfo_helper_entry_guards(control_connection_t *conn, int entries_known_but_down(const or_options_t *options); void entries_retry_all(const or_options_t *options); -char *entry_guards_get_dir_info_status_str(int using_mds, +char *entry_guards_get_err_str_if_dir_info_missing(int using_mds, int num_present, int num_usable); -char *guard_selection_get_dir_info_status_str(guard_selection_t *gs, +char *guard_selection_get_err_str_if_dir_info_missing(guard_selection_t *gs, int using_mds, int num_present, int num_usable); diff --git a/src/or/nodelist.c b/src/or/nodelist.c index 6ca9bd6d4b..eae74e18b5 100644 --- a/src/or/nodelist.c +++ b/src/or/nodelist.c @@ -2325,7 +2325,7 @@ update_router_have_minimum_dir_info(void) } { /* Check entry guard dirinfo status */ - char *guard_error = entry_guards_get_dir_info_status_str(using_md, + char *guard_error = entry_guards_get_err_str_if_dir_info_missing(using_md, num_present, num_usable); if (guard_error) { diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c index 472383f382..f9d981953d 100644 --- a/src/test/test_entrynodes.c +++ b/src/test/test_entrynodes.c @@ -1643,7 +1643,8 @@ test_entry_guard_manage_primary(void *arg) { /* Check that we have all required dirinfo for the primaries (that's done * in big_fake_network_setup()) */ - char *dir_info_str = guard_selection_get_dir_info_status_str(gs, 0, 0, 0); + char *dir_info_str = + guard_selection_get_err_str_if_dir_info_missing(gs, 0, 0, 0); tt_assert(!dir_info_str); /* Now artificially remove the first primary's descriptor and re-check */ @@ -1652,7 +1653,7 @@ test_entry_guard_manage_primary(void *arg) /* Change the first primary's identity digest so that the mocked functions * can't find its descriptor */ memset(first_primary->identity, 9, sizeof(first_primary->identity)); - dir_info_str = guard_selection_get_dir_info_status_str(gs, 1, 2, 3); + dir_info_str =guard_selection_get_err_str_if_dir_info_missing(gs, 1, 2, 3); tt_str_op(dir_info_str, OP_EQ, "We're missing descriptors for 1/2 of our primary entry guards " "(total microdescriptors: 2/3).");