[or-cvs] [tor/master 1/2] Make the controller act more usefully when GETINFO fails
nickm at torproject.org
nickm at torproject.org
Mon Jul 19 09:11:20 UTC 2010
Author: Nick Mathewson <nickm at torproject.org>
Date: Sun, 18 Jul 2010 17:05:58 +0200
Subject: Make the controller act more usefully when GETINFO fails
Commit: 0b4b51314f5cb0242e7a8fd3b87bc800cd04eacc
Right now it says "552 internal error" because there's no way for
getinfo_helper_*() countries to specify an error message. This
patch changes the getinfo_helper_*() interface, and makes most of the
getinfo helpers give useful error messages in response to failures.
This should prevent recurrences of bug 1699, where a missing GeoIPFile
line in the torrc made GETINFO ip-to-county/* fail in a "not obvious
how to fix" way.
---
changes/bug1699 | 4 ++++
src/or/circuitbuild.c | 5 ++++-
src/or/config.c | 4 +++-
src/or/control.c | 48 +++++++++++++++++++++++++++++++-----------------
src/or/geoip.c | 9 +++++++--
src/or/hibernate.c | 4 +++-
src/or/networkstatus.c | 7 +++++--
src/or/or.h | 18 ++++++++++++------
src/or/policies.c | 4 +++-
9 files changed, 72 insertions(+), 31 deletions(-)
create mode 100644 changes/bug1699
diff --git a/changes/bug1699 b/changes/bug1699
new file mode 100644
index 0000000..5d114cd
--- /dev/null
+++ b/changes/bug1699
@@ -0,0 +1,4 @@
+ o Minor features:
+ - Have the controller interface give a more useful message than
+ "Internal Error" in response to failed GETINFO requests.
+
diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 4090054..33f208a 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -4240,9 +4240,12 @@ entry_guards_update_state(or_state_t *state)
* */
int
getinfo_helper_entry_guards(control_connection_t *conn,
- const char *question, char **answer)
+ const char *question, char **answer,
+ const char **errmsg)
{
(void) conn;
+ (void) errmsg;
+
if (!strcmp(question,"entry-guards") ||
!strcmp(question,"helper-nodes")) {
smartlist_t *sl = smartlist_create();
diff --git a/src/or/config.c b/src/or/config.c
index 7dee8ff..70a3ee6 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -5060,9 +5060,11 @@ remove_file_if_very_old(const char *fname, time_t now)
* types. */
int
getinfo_helper_config(control_connection_t *conn,
- const char *question, char **answer)
+ const char *question, char **answer,
+ const char **errmsg)
{
(void) conn;
+ (void) errmsg;
if (!strcmp(question, "config/names")) {
smartlist_t *sl = smartlist_create();
int i;
diff --git a/src/or/control.c b/src/or/control.c
index ea6ce4c..ab17bec 100644
--- a/src/or/control.c
+++ b/src/or/control.c
@@ -1296,7 +1296,7 @@ handle_control_mapaddress(control_connection_t *conn, uint32_t len,
* trivial-to-implement questions. */
static int
getinfo_helper_misc(control_connection_t *conn, const char *question,
- char **answer)
+ char **answer, const char **errmsg)
{
(void) conn;
if (!strcmp(question, "version")) {
@@ -1316,15 +1316,19 @@ getinfo_helper_misc(control_connection_t *conn, const char *question,
*answer = tor_strdup("VERBOSE_NAMES EXTENDED_EVENTS");
} else if (!strcmp(question, "address")) {
uint32_t addr;
- if (router_pick_published_address(get_options(), &addr) < 0)
+ if (router_pick_published_address(get_options(), &addr) < 0) {
+ *errmsg = "Address unknown";
return -1;
+ }
*answer = tor_dup_ip(addr);
} else if (!strcmp(question, "dir-usage")) {
*answer = directory_dump_request_log();
} else if (!strcmp(question, "fingerprint")) {
routerinfo_t *me = router_get_my_routerinfo();
- if (!me)
+ if (!me) {
+ *errmsg = "No routerdesc known; am I really a server?";
return -1;
+ }
*answer = tor_malloc(HEX_DIGEST_LEN+1);
base16_encode(*answer, HEX_DIGEST_LEN+1, me->cache_info.identity_digest,
DIGEST_LEN);
@@ -1385,7 +1389,8 @@ munge_extrainfo_into_routerinfo(const char *ri_body, signed_descriptor_t *ri,
* directory information. */
static int
getinfo_helper_dir(control_connection_t *control_conn,
- const char *question, char **answer)
+ const char *question, char **answer,
+ const char **errmsg)
{
(void) control_conn;
if (!strcmpstart(question, "desc/id/")) {
@@ -1462,6 +1467,7 @@ getinfo_helper_dir(control_connection_t *control_conn,
log_warn(LD_CONTROL, "getinfo '%s': %s", question, msg);
smartlist_free(descs);
tor_free(url);
+ *errmsg = msg;
return -1;
}
SMARTLIST_FOREACH(descs, signed_descriptor_t *, sd,
@@ -1558,7 +1564,8 @@ getinfo_helper_dir(control_connection_t *control_conn,
* current states of things we send events about. */
static int
getinfo_helper_events(control_connection_t *control_conn,
- const char *question, char **answer)
+ const char *question, char **answer,
+ const char **errmsg)
{
(void) control_conn;
if (!strcmp(question, "circuit-status")) {
@@ -1759,8 +1766,10 @@ getinfo_helper_events(control_connection_t *control_conn,
}
} else if (!strcmp(question, "status/clients-seen")) {
const char *bridge_stats = geoip_get_bridge_stats_controller(time(NULL));
- if (!bridge_stats)
+ if (!bridge_stats) {
+ *errmsg = "No bridge-client stats available";
return -1;
+ }
*answer = tor_strdup(bridge_stats);
} else {
return 0;
@@ -1771,12 +1780,14 @@ getinfo_helper_events(control_connection_t *control_conn,
/** Callback function for GETINFO: on a given control connection, try to
* answer the question <b>q</b> and store the newly-allocated answer in
- * *<b>a</b>. If an internal error occurs, return -1. On success, or if
- * the key is not recognized, return 0. Do not set <b>a</b> if the key is
- * not recognized.
+ * *<b>a</b>. If an internal error occurs, return -1 and optionally set
+ * *<b>error_out</b> to point to an error message to be delivered to the
+ * controller. On success, _or if the key is not recognized_, return 0. Do not
+ * set <b>a</b> if the key is not recognized.
*/
typedef int (*getinfo_helper_t)(control_connection_t *,
- const char *q, char **a);
+ const char *q, char **a,
+ const char **error_out);
/** A single item for the GETINFO question-to-answer-function table. */
typedef struct getinfo_item_t {
@@ -1911,7 +1922,8 @@ list_getinfo_options(void)
* internal error. */
static int
handle_getinfo_helper(control_connection_t *control_conn,
- const char *question, char **answer)
+ const char *question, char **answer,
+ const char **err_out)
{
int i;
*answer = NULL; /* unrecognized key by default */
@@ -1924,7 +1936,7 @@ handle_getinfo_helper(control_connection_t *control_conn,
match = !strcmp(question, getinfo_items[i].varname);
if (match) {
tor_assert(getinfo_items[i].fn);
- return getinfo_items[i].fn(control_conn, question, answer);
+ return getinfo_items[i].fn(control_conn, question, answer, err_out);
}
}
@@ -1946,10 +1958,12 @@ handle_control_getinfo(control_connection_t *conn, uint32_t len,
smartlist_split_string(questions, body, " ",
SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0);
- SMARTLIST_FOREACH(questions, const char *, q,
- {
- if (handle_getinfo_helper(conn, q, &ans) < 0) {
- connection_write_str_to_buf("551 Internal error\r\n", conn);
+ SMARTLIST_FOREACH_BEGIN(questions, const char *, q) {
+ const char *errmsg = NULL;
+ if (handle_getinfo_helper(conn, q, &ans, &errmsg) < 0) {
+ if (!errmsg)
+ errmsg = "Internal error";
+ connection_printf_to_buf(conn, "551 %s\r\n", errmsg);
goto done;
}
if (!ans) {
@@ -1958,7 +1972,7 @@ handle_control_getinfo(control_connection_t *conn, uint32_t len,
smartlist_add(answers, tor_strdup(q));
smartlist_add(answers, ans);
}
- });
+ } SMARTLIST_FOREACH_END(q);
if (smartlist_len(unrecognized)) {
for (i=0; i < smartlist_len(unrecognized)-1; ++i)
connection_printf_to_buf(conn,
diff --git a/src/or/geoip.c b/src/or/geoip.c
index ad28a77..7f6cf79 100644
--- a/src/or/geoip.c
+++ b/src/or/geoip.c
@@ -1318,10 +1318,15 @@ geoip_entry_stats_write(time_t now)
/** Helper used to implement GETINFO ip-to-country/... controller command. */
int
getinfo_helper_geoip(control_connection_t *control_conn,
- const char *question, char **answer)
+ const char *question, char **answer,
+ const char **errmsg)
{
(void)control_conn;
- if (geoip_is_loaded() && !strcmpstart(question, "ip-to-country/")) {
+ if (!geoip_is_loaded()) {
+ *errmsg = "GeoIP data not loaded";
+ return -1;
+ }
+ if (!strcmpstart(question, "ip-to-country/")) {
int c;
uint32_t ip;
struct in_addr in;
diff --git a/src/or/hibernate.c b/src/or/hibernate.c
index 3c52a31..6b512ea 100644
--- a/src/or/hibernate.c
+++ b/src/or/hibernate.c
@@ -863,9 +863,11 @@ consider_hibernation(time_t now)
* NULL. */
int
getinfo_helper_accounting(control_connection_t *conn,
- const char *question, char **answer)
+ const char *question, char **answer,
+ const char **errmsg)
{
(void) conn;
+ (void) errmsg;
if (!strcmp(question, "accounting/enabled")) {
*answer = tor_strdup(accounting_is_enabled(get_options()) ? "1" : "0");
} else if (!strcmp(question, "accounting/hibernating")) {
diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index 49bc805..4834d13 100644
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -2124,7 +2124,8 @@ networkstatus_parse_flavor_name(const char *flavname)
* ORs. Return 0 on success, -1 on unrecognized question format. */
int
getinfo_helper_networkstatus(control_connection_t *conn,
- const char *question, char **answer)
+ const char *question, char **answer,
+ const char **errmsg)
{
routerstatus_t *status;
(void) conn;
@@ -2148,8 +2149,10 @@ getinfo_helper_networkstatus(control_connection_t *conn,
} else if (!strcmpstart(question, "ns/id/")) {
char d[DIGEST_LEN];
- if (base16_decode(d, DIGEST_LEN, question+6, strlen(question+6)))
+ if (base16_decode(d, DIGEST_LEN, question+6, strlen(question+6))) {
+ *errmsg = "Data not decodeable as hex";
return -1;
+ }
status = router_get_consensus_status_by_id(d);
} else if (!strcmpstart(question, "ns/name/")) {
status = router_get_consensus_status_by_nickname(question+8, 0);
diff --git a/src/or/or.h b/src/or/or.h
index c9c2f41..3418f53 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -3004,7 +3004,8 @@ routerinfo_t *choose_random_entry(cpath_build_state_t *state);
int entry_guards_parse_state(or_state_t *state, int set, char **msg);
void entry_guards_update_state(or_state_t *state);
int getinfo_helper_entry_guards(control_connection_t *conn,
- const char *question, char **answer);
+ const char *question, char **answer,
+ const char **errmsg);
void clear_bridge_list(void);
int routerinfo_is_a_configured_bridge(routerinfo_t *ri);
@@ -3361,7 +3362,8 @@ int or_state_save(time_t now);
int options_need_geoip_info(or_options_t *options, const char **reason_out);
int getinfo_helper_config(control_connection_t *conn,
- const char *question, char **answer);
+ const char *question, char **answer,
+ const char **errmsg);
const char *tor_get_digests(void);
uint32_t get_effective_bwrate(or_options_t *options);
@@ -4162,7 +4164,8 @@ char *geoip_get_client_history_bridge(time_t now,
geoip_client_action_t action);
char *geoip_get_request_history(time_t now, geoip_client_action_t action);
int getinfo_helper_geoip(control_connection_t *control_conn,
- const char *question, char **answer);
+ const char *question, char **answer,
+ const char **errmsg);
void geoip_free_all(void);
/** Directory requests that we are measuring can be either direct or
@@ -4220,7 +4223,8 @@ void hibernate_begin_shutdown(void);
int we_are_hibernating(void);
void consider_hibernation(time_t now);
int getinfo_helper_accounting(control_connection_t *conn,
- const char *question, char **answer);
+ const char *question, char **answer,
+ const char **errmsg);
void accounting_set_bandwidth_usage_from_state(or_state_t *state);
/********************************* main.c ***************************/
@@ -4397,7 +4401,8 @@ int32_t get_net_param_from_list(smartlist_t *net_params, const char *name,
int32_t networkstatus_get_param(networkstatus_t *ns, const char *param_name,
int32_t default_val);
int getinfo_helper_networkstatus(control_connection_t *conn,
- const char *question, char **answer);
+ const char *question, char **answer,
+ const char **errmsg);
int32_t networkstatus_get_bw_weight(networkstatus_t *ns, const char *weight,
int32_t default_val);
const char *networkstatus_get_flavor_name(consensus_flavor_t flav);
@@ -4504,7 +4509,8 @@ void policies_set_router_exitpolicy_to_reject_all(routerinfo_t *exitrouter);
int exit_policy_is_general_exit(smartlist_t *policy);
int policy_is_reject_star(const smartlist_t *policy);
int getinfo_helper_policies(control_connection_t *conn,
- const char *question, char **answer);
+ const char *question, char **answer,
+ const char **errmsg);
int policy_write_item(char *buf, size_t buflen, addr_policy_t *item,
int format_for_desc);
diff --git a/src/or/policies.c b/src/or/policies.c
index 90e159a..f5c02a6 100644
--- a/src/or/policies.c
+++ b/src/or/policies.c
@@ -1288,9 +1288,11 @@ cleanup:
* about "exit-policy/..." */
int
getinfo_helper_policies(control_connection_t *conn,
- const char *question, char **answer)
+ const char *question, char **answer,
+ const char **errmsg)
{
(void) conn;
+ (void) errmsg;
if (!strcmp(question, "exit-policy/default")) {
*answer = tor_strdup(DEFAULT_EXIT_POLICY);
}
--
1.7.1
More information about the tor-commits
mailing list