Better messages when POSTDESCRIPTOR fails

svn:r3989

4 files changed, 34 insertions, 16 deletions

diff --git a/doc/control-spec.txt b/doc/control-spec.txt
index e89a460c23..decff5280e 100644
--- a/doc/control-spec.txt
+++ b/doc/control-spec.txt
@@ -382,9 +382,10 @@ the message.
   The descriptor, when parsed, must contain a number of well-specified
   fields, including fields for its nickname and identity.
 
-  If there is an error in parsing the descriptor, or if the server rejects
-  the descriptor for any reason, the server must send an appropriate error
-  message.
+  If there is an error in parsing the descriptor, the server must send an
+  appropriate error message.  If the descriptor is well-formed but the server
+  chooses not to add it, it must reply with a DONE message whose body
+  explains why the server was not added.
 
 3.17 FRAGMENTHEADER (Type 0x0010)
 
diff --git a/src/or/control.c b/src/or/control.c
index b59940734e..1fc0f7017a 100644
--- a/src/or/control.c
+++ b/src/or/control.c
@@ -230,6 +230,8 @@ send_control_done(connection_t *conn)
 
 static void send_control_done2(connection_t *conn, const char *msg, size_t len)
 {
+  if (len==0)
+    len = strlen(msg);
   send_control_message(conn, CONTROL_CMD_DONE, len, msg);
 }
 
@@ -760,10 +762,16 @@ static int
 handle_control_postdescriptor(connection_t *conn, uint32_t len,
                               const char *body)
 {
-  if (router_load_single_router(body)<0) {
-    /* XXXX a more specific error would be nice. */
-    send_control_error(conn,ERR_UNSPECIFIED,
-                       "Could not parse descriptor or add it");
+  const char *msg;
+  switch (router_load_single_router(body, &msg)) {
+  case -1:
+    send_control_error(conn,ERR_SYNTAX,msg?msg: "Could not parse descriptor");
+    break;
+  case 0:
+    send_control_done2(conn,msg?msg: "Descriptor not added",0);
+    break;
+  case 1:
+    send_control_done(conn);
     return 0;
   }
 
diff --git a/src/or/or.h b/src/or/or.h
index 3b9dcbb48d..6984ddd72a 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -1796,7 +1796,7 @@ void free_trusted_dir_servers(void);
 routerinfo_t *routerinfo_copy(const routerinfo_t *router);
 void router_mark_as_down(const char *digest);
 void routerlist_remove_old_routers(int age);
-int router_load_single_router(const char *s);
+int router_load_single_router(const char *s, const char **msg);
 int router_load_routerlist_from_directory(const char *s,crypto_pk_env_t *pkey,
                                         int dir_is_recent, int dir_is_cached);
 addr_policy_result_t router_compare_addr_to_addr_policy(uint32_t addr,
diff --git a/src/or/routerlist.c b/src/or/routerlist.c
index d8dc2873ed..0de1eb9aad 100644
--- a/src/or/routerlist.c
+++ b/src/or/routerlist.c
@@ -769,9 +769,11 @@ void router_mark_as_down(const char *digest) {
  * their pointers to <b>router</b> after invoking this function; <b>router</b>
  * will either be inserted into the routerlist or freed.  Returns 0 if the
  * router was added; -1 if it was not.
+ *
+ * DOCDOC msg
  */
 static int
-router_add_to_routerlist(routerinfo_t *router) {
+router_add_to_routerlist(routerinfo_t *router, const char **msg) {
   int i;
   routerinfo_t *r;
   char id_digest[DIGEST_LEN];
@@ -797,11 +799,12 @@ router_add_to_routerlist(routerinfo_t *router) {
         smartlist_set(routerlist->routers, i, router);
         return 0;
       } else {
-        log_fn(LOG_DEBUG, "Skipping old entry for router '%s'",
+        log_fn(LOG_DEBUG, "Skipping not-new descriptor for router '%s'",
                router->nickname);
         /* Update the is_running status to whatever we were told. */
         r->is_running = router->is_running;
         routerinfo_free(router);
+        if (msg) *msg = "Router descriptor was not new.";
         return -1;
       }
     } else if (!strcasecmp(router->nickname, r->nickname)) {
@@ -827,6 +830,7 @@ router_add_to_routerlist(routerinfo_t *router) {
         log_fn(LOG_DEBUG, "Skipping unverified entry for verified router '%s'",
                router->nickname);
         routerinfo_free(router);
+        if (msg) *msg = "Already have verified router with different key and same nickname";
         return -1;
       }
     }
@@ -865,15 +869,19 @@ routerlist_remove_old_routers(int age)
 }
 
 /*
- * Code to parse router descriptors and directories.
+ * Code to parse a single router descriptors and insert it into the
+ * directory.  Return -1 if the descriptor was ill-formed; 0 if the
+ * descriptor was well-formed but could not be added; and 1 if the
+ * descriptor was added.
  */
 int
-router_load_single_router(const char *s)
+router_load_single_router(const char *s, const char **msg)
 {
   routerinfo_t *ri;
 
   if (!(ri = router_parse_entry_from_string(s, NULL))) {
     log_fn(LOG_WARN, "Error parsing router descriptor; dropping.");
+    if (msg) *msg = "Couldn't parse router descriptor";
     return -1;
   }
   if (routerlist && routerlist->running_routers) {
@@ -883,9 +891,10 @@ router_load_single_router(const char *s)
                                         rr->running_routers,
                                         rr->is_running_routers_format);
   }
-  if (router_add_to_routerlist(ri)<0) {
+  if (router_add_to_routerlist(ri, msg)<0) {
     log_fn(LOG_WARN, "Couldn't add router to list; dropping.");
-    return -1;
+    if (msg && !*msg) *msg = "Couldn't add router to list.";
+    return 0;
   } else {
     smartlist_t *changed = smartlist_create();
     smartlist_add(changed, ri);
@@ -893,7 +902,7 @@ router_load_single_router(const char *s)
     smartlist_free(changed);
   }
   log_fn(LOG_DEBUG, "Added router to list");
-  return 0;
+  return 1;
 }
 
 /** Add to the current routerlist each router stored in the
@@ -922,7 +931,7 @@ int router_load_routerlist_from_directory(const char *s,
     smartlist_t *changed = smartlist_create();
     SMARTLIST_FOREACH(new_list->routers, routerinfo_t *, r,
     {
-      if (router_add_to_routerlist(r)==0)
+      if (router_add_to_routerlist(r,NULL)==0)
         smartlist_add(changed, r);
     });
     smartlist_clear(new_list->routers);