#21349: Refactor long functions in entrynodes.c #173
Conversation
src/or/entrynodes.c
Outdated
| @@ -142,6 +142,9 @@ | |||
| #include "or/node_st.h" | |||
| #include "or/origin_circuit_st.h" | |||
|
|
|||
| #define REMOVE_UNLISTED_GUARDS_AFTER \ | |||
Hmm, not sure if I like this macro doing multiplications and casting to time_t. Suggestion: Let's modify get_remove_unlisted_guards_after_days() to return exactly what we want, and let's do the multiplication in the log of sampled_guards_prune_obsolete_entries(). What do you think?
In 64685c3 I made a function that returns the exact number we need here.
src/or/entrynodes.c
Outdated
| * some guards as listed or unlisted, and removing expired guards. */ | ||
| STATIC void | ||
| sampled_guards_update_from_consensus(guard_selection_t *gs) | ||
| /** Enumerate <b>sampled_entry_guards</b> smartlist in <b>gs</b>. |
Not exactly the right function documentation style. For multi-line comments perhaps leave the /** line empty. See how other functions do it.
Fixed in 0977049.
LGTM.
src/or/entrynodes.c
Outdated
| } | ||
| log_info(LD_GUARD, "Updating sampled guard status based on received " | ||
| "consensus."); | ||
| tor_assert(n_changes); |
Suggestion: Instead of taking an n_changes pointer argument, let's switch this function (and the other one) from void to int, and have it return the value of n_changes. I think that will be cleaner, since there won't be a need to do pointer operations.
Did so (with size_t) in eea7c81.
LGTM.
src/or/entrynodes.c
Outdated
| int n_changes = 0; | ||
|
|
||
| /* First: Update listed/unlisted. */ | ||
| sampled_guards_update_listedness(gs, &n_changes); |
Suggestion: Rename sampled_guards_update_listedness() to sampled_guards_update_consensus_presence(), since 'listedness' is not actually a word. Let me know if you like this, or you prefer something else, or prefer to leave it as is.
Renamed in 72da8b4.
LGTM.
src/or/entrynodes.c
Outdated
| /* First: Update listed/unlisted. */ | ||
| sampled_guards_update_listedness(gs, &n_changes); | ||
|
|
||
| const time_t remove_if_unlisted_since = |
Perhaps we can do these computations inside sampled_guards_prune_obsolete_entries()? Or you like them here?
I prefer sampled_guards_prune_obsolete_entries() to be parametrised by threshold values - it's more testable that way when we change our code.
Sounds good!
src/common/container.h
Outdated
| @@ -93,6 +93,24 @@ static inline void smartlist_swap(smartlist_t *sl, int idx1, int idx2) | |||
| } | |||
| } | |||
|
|
|||
| /** Return true if there is shallow equality between smartlists - | |||
Perhaps we should put this in container.c? That's where the other _eq() functions are. I'm not sure if I like functions in .h files, but it seems to be the case in container.h, so if you like it like this feel free to leave it as is.
Moved to container.c in cda8adc.
LGTM
src/common/container.h
Outdated
| */ | ||
| static inline int smartlist_shallow_eq(const smartlist_t *s1, | ||
| const smartlist_t *s2) | ||
| { |
Since we opted for a generic function here we should also check that s1 and s2 are non-NULL and return 0 if any of them are NULL.
Introduced some extra checks in cda8adc.
LGTM
src/common/container.h
Outdated
| * i.e. all indices correspond to exactly same object (pointer | ||
| * values are matching). Otherwise, return false. | ||
| */ | ||
| static inline int smartlist_shallow_eq(const smartlist_t *s1, |
Optional suggestion: We can make this function return bool instead of int.
We generally don't use bool in the codebase, so I'll rather use int, like we do pretty much always.
stdbool was only recently added to the codebase that's why it's not yet used extensively. In any case, I'm fine with int as well.
src/or/entrynodes.c
Outdated
| SAMPLE_EXCLUDE_PENDING | | ||
| flags); | ||
| if (chosen_guard == NULL) { | ||
| log_info(LD_GUARD, "Absolutely no sampled guards were available. " |
Suggestion: Perhaps we can pull this mark all guards as maybe reachable logic out of this function, and into the end of select_entry_guard_for_circuit() to make it clear that this is the end of the algorithm. We could do this in a separate commit in the end, if you like the idea.
Did so in d02b2f1.
LGTM!
| @@ -0,0 +1,6 @@ | |||
| o Code simplification and refactoring: | |||
General comment: This branch needs to be rebased to the latest master, which contains the various modularization structure improvements. Let me know if you don't want to do this, and I will do it myself.
I will do this once I address all the things you mentioned.
|
Superseded by #246. |
https://trac.torproject.org/projects/tor/ticket/21349
The text was updated successfully, but these errors were encountered: