commit db86b9194dfc6759cd731f8eedc37b7534afbf0d Author: Nick Mathewson nickm@torproject.org Date: Tue May 2 13:06:25 2017 -0400

Break connection_dir_client_reached_eof() into smaller functions

This was a >630-line function, which doesn't make anybody happy. It was also mostly composed of a bunch of if-statements that handled different directory responses differently depending on the original purpose of the directory connection. The logical refactoring here is to move the body of each switch statement into a separate handler function, and to invoke those functions from a separate switch statement.

This commit leaves whitespace mostly untouched, for ease of review. I'll reindent in the next commit. --- changes/refactor_reached_eof | 5 + src/or/directory.c | 319 +++++++++++++++++++++++++++++++++++++------ 2 files changed, 286 insertions(+), 38 deletions(-)

diff --git a/changes/refactor_reached_eof b/changes/refactor_reached_eof new file mode 100644 index 0000000..33ab931 --- /dev/null +++ b/changes/refactor_reached_eof @@ -0,0 +1,5 @@ + o Code simplification and refactoring: + + - Break up the 630-line function connection_dir_client_reached_eof() into + a dozen smaller functions. This change should help maintainability and + readability of the client directory code. diff --git a/src/or/directory.c b/src/or/directory.c index 2b9f18a..0635be1 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -2073,6 +2073,39 @@ load_downloaded_routers(const char *body, smartlist_t *which, return added; }

+/** A structure to hold arguments passed into each directory response + * handler */ +typedef struct response_handler_args_t { + int status_code; + const char *reason; + const char *body; + size_t body_len; + const char *headers; +} response_handler_args_t; + +static int handle_response_fetch_consensus(dir_connection_t *, + const response_handler_args_t *); +static int handle_response_fetch_certificate(dir_connection_t *, + const response_handler_args_t *); +static int handle_response_fetch_status_vote(dir_connection_t *, + const response_handler_args_t *); +static int handle_response_fetch_detached_signatures(dir_connection_t *, + const response_handler_args_t *); +static int handle_response_fetch_desc(dir_connection_t *, + const response_handler_args_t *); +static int handle_response_fetch_microdesc(dir_connection_t *, + const response_handler_args_t *); +static int handle_response_upload_dir(dir_connection_t *, + const response_handler_args_t *); +static int handle_response_upload_vote(dir_connection_t *, + const response_handler_args_t *); +static int handle_response_upload_signatures(dir_connection_t *, + const response_handler_args_t *); +static int handle_response_fetch_renddesc_v2(dir_connection_t *, + const response_handler_args_t *); +static int handle_response_upload_renddesc_v2(dir_connection_t *, + const response_handler_args_t *); + /** We are a client, and we've finished reading the server's * response. Parse it and act appropriately. * @@ -2098,8 +2131,6 @@ connection_dir_client_reached_eof(dir_connection_t *conn) int allow_partial = (conn->base_.purpose == DIR_PURPOSE_FETCH_SERVERDESC || conn->base_.purpose == DIR_PURPOSE_FETCH_EXTRAINFO || conn->base_.purpose == DIR_PURPOSE_FETCH_MICRODESC); - time_t now = time(NULL); - int src_code; size_t received_bytes;

received_bytes = connection_get_inbuf_len(TO_CONN(conn)); @@ -2193,6 +2224,7 @@ connection_dir_client_reached_eof(dir_connection_t *conn) "'%s:%d'. I'll try again soon.", status_code, escaped(reason), conn->base_.address, conn->base_.port); + time_t now = approx_time(); if ((rs = router_get_mutable_consensus_status_by_id(id_digest))) rs->last_dir_503_at = now; if ((ds = router_get_fallback_dirserver_by_digest(id_digest))) @@ -2260,7 +2292,77 @@ connection_dir_client_reached_eof(dir_connection_t *conn) } }

- if (conn->base_.purpose == DIR_PURPOSE_FETCH_CONSENSUS) { + int rv; + response_handler_args_t args; + memset(&args, 0, sizeof(args)); + args.status_code = status_code; + args.reason = reason; + args.body = body; + args.body_len = body_len; + args.headers = headers; + + switch (conn->base_.purpose) { + case DIR_PURPOSE_FETCH_CONSENSUS: + rv = handle_response_fetch_consensus(conn, &args); + break; + case DIR_PURPOSE_FETCH_CERTIFICATE: + rv = handle_response_fetch_certificate(conn, &args); + break; + case DIR_PURPOSE_FETCH_STATUS_VOTE: + rv = handle_response_fetch_status_vote(conn, &args); + break; + case DIR_PURPOSE_FETCH_DETACHED_SIGNATURES: + rv = handle_response_fetch_detached_signatures(conn, &args); + break; + case DIR_PURPOSE_FETCH_SERVERDESC: + case DIR_PURPOSE_FETCH_EXTRAINFO: + rv = handle_response_fetch_desc(conn, &args); + break; + case DIR_PURPOSE_FETCH_MICRODESC: + rv = handle_response_fetch_microdesc(conn, &args); + break; + case DIR_PURPOSE_FETCH_RENDDESC_V2: + rv = handle_response_fetch_renddesc_v2(conn, &args); + break; + case DIR_PURPOSE_UPLOAD_DIR: + rv = handle_response_upload_dir(conn, &args); + break; + case DIR_PURPOSE_UPLOAD_SIGNATURES: + rv = handle_response_upload_signatures(conn, &args); + break; + case DIR_PURPOSE_UPLOAD_VOTE: + rv = handle_response_upload_vote(conn, &args); + break; + case DIR_PURPOSE_UPLOAD_RENDDESC_V2: + rv = handle_response_upload_renddesc_v2(conn, &args); + break; + default: + tor_assert_nonfatal_unreached(); + rv = -1; + break; + } + tor_free(body); + tor_free(headers); + tor_free(reason); + return rv; +} + +/** + * Handler function: processes a response to a request for a networkstatus + * consensus document by checking the consensus, storing it, and marking + * router requests as reachable. + **/ +static int +handle_response_fetch_consensus(dir_connection_t *conn, + const response_handler_args_t *args) +{ + tor_assert(conn->base_.purpose == DIR_PURPOSE_FETCH_CONSENSUS); + const int status_code = args->status_code; + const char *body = args->body; + const size_t body_len = args->body_len; + const char *reason = args->reason; + const time_t now = approx_time(); + int r; const char *flavname = conn->requested_resource; if (status_code != 200) { @@ -2270,7 +2372,6 @@ connection_dir_client_reached_eof(dir_connection_t *conn) "'%s:%d' while fetching consensus directory.", status_code, escaped(reason), conn->base_.address, conn->base_.port); - tor_free(body); tor_free(headers); tor_free(reason); networkstatus_consensus_download_failed(status_code, flavname); return -1; } @@ -2282,7 +2383,6 @@ connection_dir_client_reached_eof(dir_connection_t *conn) "Unable to load %s consensus directory downloaded from " "server '%s:%d'. I'll try again soon.", flavname, conn->base_.address, conn->base_.port); - tor_free(body); tor_free(headers); tor_free(reason); networkstatus_consensus_download_failed(0, flavname); return -1; } @@ -2300,17 +2400,31 @@ connection_dir_client_reached_eof(dir_connection_t *conn) networkstatus_get_latest_consensus_by_flavor(FLAV_NS)); } log_info(LD_DIR, "Successfully loaded consensus."); - }

- if (conn->base_.purpose == DIR_PURPOSE_FETCH_CERTIFICATE) { - if (status_code != 200) { + return 0; +} + +/** + * Handler function: processes a response to a request for one or more + * authority certificates + **/ +static int +handle_response_fetch_certificate(dir_connection_t *conn, + const response_handler_args_t *args) +{ + tor_assert(conn->base_.purpose == DIR_PURPOSE_FETCH_CERTIFICATE); + const int status_code = args->status_code; + const char *reason = args->reason; + const char *body = args->body; + const size_t body_len = args->body_len; + + if (status_code != 200) { log_warn(LD_DIR, "Received http status code %d (%s) from server " "'%s:%d' while fetching "/tor/keys/%s".", status_code, escaped(reason), conn->base_.address, conn->base_.port, conn->requested_resource); connection_dir_download_cert_failed(conn, status_code); - tor_free(body); tor_free(headers); tor_free(reason); return -1; } log_info(LD_DIR,"Received authority certificates (body size %d) from " @@ -2321,7 +2435,7 @@ connection_dir_client_reached_eof(dir_connection_t *conn) * Tell trusted_dirs_load_certs_from_string() whether it was by fp * or fp-sk pair. */ - src_code = -1; + int src_code = -1; if (!strcmpstart(conn->requested_resource, "fp/")) { src_code = TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST; } else if (!strcmpstart(conn->requested_resource, "fp-sk/")) { @@ -2336,6 +2450,7 @@ connection_dir_client_reached_eof(dir_connection_t *conn) * ones got flushed to disk so it's safe to call this on them */ connection_dir_download_cert_failed(conn, status_code); } else { + time_t now = approx_time(); directory_info_has_arrived(now, 0, 0); log_info(LD_DIR, "Successfully loaded certificates from fetch."); } @@ -2346,8 +2461,24 @@ connection_dir_client_reached_eof(dir_connection_t *conn) conn->requested_resource); connection_dir_download_cert_failed(conn, status_code); } - } - if (conn->base_.purpose == DIR_PURPOSE_FETCH_STATUS_VOTE) { + return 0; +} + + +/** + * Handler function: processes a response to a request for an authority's + * current networkstatus vote. + **/ +static int +handle_response_fetch_status_vote(dir_connection_t *conn, + const response_handler_args_t *args) +{ + tor_assert(conn->base_.purpose == DIR_PURPOSE_FETCH_STATUS_VOTE); + const int status_code = args->status_code; + const char *reason = args->reason; + const char *body = args->body; + const size_t body_len = args->body_len; + const char *msg; int st; log_info(LD_DIR,"Got votes (body size %d) from server %s:%d", @@ -2358,7 +2489,6 @@ connection_dir_client_reached_eof(dir_connection_t *conn) "'%s:%d' while fetching "/tor/status-vote/next/%s.z".", status_code, escaped(reason), conn->base_.address, conn->base_.port, conn->requested_resource); - tor_free(body); tor_free(headers); tor_free(reason); return -1; } dirvote_add_vote(body, &msg, &st); @@ -2367,8 +2497,24 @@ connection_dir_client_reached_eof(dir_connection_t *conn) } else { log_info(LD_DIR, "Added vote(s) successfully [msg: %s]", msg); } - } - if (conn->base_.purpose == DIR_PURPOSE_FETCH_DETACHED_SIGNATURES) { + + return 0; +} + +/** + * Handler function: processes a response to a request for the signatures + * that an authority knows about on a given consensus. + **/ +static int +handle_response_fetch_detached_signatures(dir_connection_t *conn, + const response_handler_args_t *args) +{ + tor_assert(conn->base_.purpose == DIR_PURPOSE_FETCH_DETACHED_SIGNATURES); + const int status_code = args->status_code; + const char *reason = args->reason; + const char *body = args->body; + const size_t body_len = args->body_len; + const char *msg = NULL; log_info(LD_DIR,"Got detached signatures (body size %d) from server %s:%d", (int)body_len, conn->base_.address, conn->base_.port); @@ -2378,17 +2524,31 @@ connection_dir_client_reached_eof(dir_connection_t *conn) ""/tor/status-vote/next/consensus-signatures.z".", status_code, escaped(reason), conn->base_.address, conn->base_.port); - tor_free(body); tor_free(headers); tor_free(reason); return -1; } if (dirvote_add_signatures(body, conn->base_.address, &msg)<0) { log_warn(LD_DIR, "Problem adding detached signatures from %s:%d: %s", conn->base_.address, conn->base_.port, msg?msg:"???"); } - }

- if (conn->base_.purpose == DIR_PURPOSE_FETCH_SERVERDESC || - conn->base_.purpose == DIR_PURPOSE_FETCH_EXTRAINFO) { + return 0; +} + +/** + * Handler function: processes a response to a request for a group of server + * descriptors or an extrainfo documents. + **/ +static int +handle_response_fetch_desc(dir_connection_t *conn, + const response_handler_args_t *args) +{ + tor_assert(conn->base_.purpose == DIR_PURPOSE_FETCH_SERVERDESC || + conn->base_.purpose == DIR_PURPOSE_FETCH_EXTRAINFO); + const int status_code = args->status_code; + const char *reason = args->reason; + const char *body = args->body; + const size_t body_len = args->body_len; + int was_ei = conn->base_.purpose == DIR_PURPOSE_FETCH_EXTRAINFO; smartlist_t *which = NULL; int n_asked_for = 0; @@ -2425,7 +2585,6 @@ connection_dir_client_reached_eof(dir_connection_t *conn) SMARTLIST_FOREACH(which, char *, cp, tor_free(cp)); smartlist_free(which); } - tor_free(body); tor_free(headers); tor_free(reason); return dir_okay ? 0 : -1; } /* Learn the routers, assuming we requested by fingerprint or "all" @@ -2449,8 +2608,10 @@ connection_dir_client_reached_eof(dir_connection_t *conn) // descriptor_digests, conn->router_purpose); if (load_downloaded_routers(body, which, descriptor_digests, conn->router_purpose, - conn->base_.address)) + conn->base_.address)) { + time_t now = approx_time(); directory_info_has_arrived(now, 0, 0); + } } } if (which) { /* mark remaining ones as failed */ @@ -2468,8 +2629,24 @@ connection_dir_client_reached_eof(dir_connection_t *conn) } if (directory_conn_is_self_reachability_test(conn)) router_dirport_found_reachable(); - } - if (conn->base_.purpose == DIR_PURPOSE_FETCH_MICRODESC) { + + return 0; +} + +/** + * Handler function: processes a response to a request for a group of + * microdescriptors + **/ +static int +handle_response_fetch_microdesc(dir_connection_t *conn, + const response_handler_args_t *args) +{ + tor_assert(conn->base_.purpose == DIR_PURPOSE_FETCH_MICRODESC); + const int status_code = args->status_code; + const char *reason = args->reason; + const char *body = args->body; + const size_t body_len = args->body_len; + smartlist_t *which = NULL; log_info(LD_DIR,"Received answer to microdescriptor request (status %d, " "body size %d) from server '%s:%d'", @@ -2490,10 +2667,10 @@ connection_dir_client_reached_eof(dir_connection_t *conn) dir_microdesc_download_failed(which, status_code); SMARTLIST_FOREACH(which, char *, cp, tor_free(cp)); smartlist_free(which); - tor_free(body); tor_free(headers); tor_free(reason); return 0; } else { smartlist_t *mds; + time_t now = approx_time(); mds = microdescs_add_to_cache(get_microdesc_cache(), body, body+body_len, SAVED_NOWHERE, 0, now, which); @@ -2510,9 +2687,23 @@ connection_dir_client_reached_eof(dir_connection_t *conn) smartlist_free(which); smartlist_free(mds); } - }

- if (conn->base_.purpose == DIR_PURPOSE_UPLOAD_DIR) { + return 0; +} + +/** + * Handler function: processes a response to a POST request to upload our + * router descriptor. + **/ +static int +handle_response_upload_dir(dir_connection_t *conn, + const response_handler_args_t *args) +{ + tor_assert(conn->base_.purpose == DIR_PURPOSE_UPLOAD_DIR); + const int status_code = args->status_code; + const char *reason = args->reason; + const char *headers = args->headers; + switch (status_code) { case 200: { dir_server_t *ds = @@ -2563,9 +2754,22 @@ connection_dir_client_reached_eof(dir_connection_t *conn) } /* return 0 in all cases, since we don't want to mark any * dirservers down just because they don't like us. */ - }

- if (conn->base_.purpose == DIR_PURPOSE_UPLOAD_VOTE) { + return 0; +} + +/** + * Handler function: processes a response to POST request to upload our + * own networkstatus vote. + **/ +static int +handle_response_upload_vote(dir_connection_t *conn, + const response_handler_args_t *args) +{ + tor_assert(conn->base_.purpose == DIR_PURPOSE_UPLOAD_VOTE); + const int status_code = args->status_code; + const char *reason = args->reason; + switch (status_code) { case 200: { log_notice(LD_DIR,"Uploaded a vote to dirserver %s:%d", @@ -2587,9 +2791,21 @@ connection_dir_client_reached_eof(dir_connection_t *conn) } /* return 0 in all cases, since we don't want to mark any * dirservers down just because they don't like us. */ - } + return 0; +} + +/** + * Handler function: processes a response to POST request to upload our + * view of the signatures on the current consensus. + **/ +static int +handle_response_upload_signatures(dir_connection_t *conn, + const response_handler_args_t *args) +{ + tor_assert(conn->base_.purpose == DIR_PURPOSE_UPLOAD_SIGNATURES); + const int status_code = args->status_code; + const char *reason = args->reason;

- if (conn->base_.purpose == DIR_PURPOSE_UPLOAD_SIGNATURES) { switch (status_code) { case 200: { log_notice(LD_DIR,"Uploaded signature(s) to dirserver %s:%d", @@ -2611,10 +2827,25 @@ connection_dir_client_reached_eof(dir_connection_t *conn) } /* return 0 in all cases, since we don't want to mark any * dirservers down just because they don't like us. */ - }

- if (conn->base_.purpose == DIR_PURPOSE_FETCH_RENDDESC_V2) { - #define SEND_HS_DESC_FAILED_EVENT(reason) ( \ + return 0; +} + +/** + * Handler function: processes a response to a request for a v2 hidden service + * descriptor. + **/ +static int +handle_response_fetch_renddesc_v2(dir_connection_t *conn, + const response_handler_args_t *args) +{ + tor_assert(conn->base_.purpose == DIR_PURPOSE_FETCH_RENDDESC_V2); + const int status_code = args->status_code; + const char *reason = args->reason; + const char *body = args->body; + const size_t body_len = args->body_len; + + #define SEND_HS_DESC_FAILED_EVENT(reason) ( \ control_event_hs_descriptor_failed(conn->rend_data, \ conn->identity_digest, \ reason) ) @@ -2689,10 +2920,23 @@ connection_dir_client_reached_eof(dir_connection_t *conn) SEND_HS_DESC_FAILED_CONTENT(); break; } - }

- if (conn->base_.purpose == DIR_PURPOSE_UPLOAD_RENDDESC_V2) { - #define SEND_HS_DESC_UPLOAD_FAILED_EVENT(reason) ( \ + return 0; +} + +/** + * Handler function: processes a response to a POST request to upload a v2 + * hidden service descriptor. + **/ +static int +handle_response_upload_renddesc_v2(dir_connection_t *conn, + const response_handler_args_t *args) +{ + tor_assert(conn->base_.purpose == DIR_PURPOSE_UPLOAD_RENDDESC_V2); + const int status_code = args->status_code; + const char *reason = args->reason; + + #define SEND_HS_DESC_UPLOAD_FAILED_EVENT(reason) ( \ control_event_hs_descriptor_upload_failed( \ conn->identity_digest, \ rend_data_get_address(conn->rend_data), \ @@ -2726,8 +2970,7 @@ connection_dir_client_reached_eof(dir_connection_t *conn) SEND_HS_DESC_UPLOAD_FAILED_EVENT("UNEXPECTED"); break; } - } - tor_free(body); tor_free(headers); tor_free(reason); + return 0; }