Make return code from router_add_to_routerlist a nice sensible enum. Based on patch from Sebastian.

svn:r17656

src/or/directory.c

@@ -3061,34 +3061,34 @@ directory_handle_command_post(dir_connection_t *conn, const char *headers,
 
   if (authdir_mode_handles_descs(options, -1) &&
       !strcmp(url,"/tor/")) { /* server descriptor post */
-    const char *msg = NULL;
+    const char *msg = "[None]";
     uint8_t purpose = authdir_mode_bridge(options) ?
                       ROUTER_PURPOSE_BRIDGE : ROUTER_PURPOSE_GENERAL;
-    int r = dirserv_add_multiple_descriptors(body, purpose,
+    was_router_added_t r = dirserv_add_multiple_descriptors(body, purpose,
                                              conn->_base.address, &msg);
     tor_assert(msg);
-    if (r > 0)
+    if (WRA_WAS_ADDED(r))
       dirserv_get_directory(); /* rebuild and write to disk */
-    switch (r) {
+
-      case -1:
+    if (r == ROUTER_ADDED_NOTIFY_GENERATOR) {
-        log_notice(LD_DIRSERV,
+      /* Accepted with a message. */
+      log_info(LD_DIRSERV,
+               "Problematic router descriptor or extra-info from %s "
+               "(\"%s\").",
+               conn->_base.address, msg);
+      write_http_status_line(conn, 400, msg);
+    } else if (r == ROUTER_ADDED_SUCCESSFULLY) {
+      write_http_status_line(conn, 200, msg);
+    } else if (WRA_WAS_OUTDATED(r)) {
+      write_http_response_header_impl(conn, -1, NULL, NULL,
+                                      "X-Descriptor-Not-New: Yes\r\n", -1);
+    } else {
+      log_info(LD_DIRSERV,
                "Rejected router descriptor or extra-info from %s "
                "(\"%s\").",
                conn->_base.address, msg);
-        /* fall through */
-      case 1:
-        /* malformed descriptor, or something wrong */
       write_http_status_line(conn, 400, msg);
-        break;
-      case 0: /* accepted but discarded */
-        write_http_response_header_impl(conn, -1, NULL, NULL,
-                                        "X-Descriptor-Not-New: Yes\r\n", -1);
-        break;
-      case 2: /* accepted */
-        write_http_status_line(conn, 200, msg);
-        break;
     }
-    goto done;
   }
 
   if (options->HSAuthoritativeDir &&

src/or/dirserv.c

@@ -564,14 +564,27 @@ authdir_wants_to_reject_router(routerinfo_t *ri, const char **msg,
   return 0;
 }
 
+/** True iff <b>a</b> is more severe than <b>b</b> */
+static int
+WRA_MORE_SEVERE(was_router_added_t a, was_router_added_t b)
+{
+  if (b == ROUTER_ADDED_SUCCESSFULLY) {
+    return a;
+  } else if (b == ROUTER_ADDED_NOTIFY_GENERATOR) {
+    return !WRA_WAS_ADDED(a);
+  } else {
+    return a < b;
+  }
+}
+
 /** As for dirserv_add_descriptor, but accepts multiple documents, and
  * returns the most severe error that occurred for any one of them. */
-int
+was_router_added_t
 dirserv_add_multiple_descriptors(const char *desc, uint8_t purpose,
                                  const char *source,
                                  const char **msg)
 {
-  int r=100; /* higher than any actual return value. */
+  int r=ROUTER_ADDED_NOTIFY_GENERATOR; /* highest possible return value. */
   int r_tmp;
   const char *msg_out;
   smartlist_t *list;
@@ -603,7 +616,7 @@ dirserv_add_multiple_descriptors(const char *desc, uint8_t purpose,
         msg_out = NULL;
         tor_assert(ri->purpose == purpose);
         r_tmp = dirserv_add_descriptor(ri, &msg_out);
-        if (r_tmp < r) {
+        if (WRA_MORE_SEVERE(r_tmp, r)) {
           r = r_tmp;
           *msg = msg_out;
         }
@@ -619,7 +632,7 @@ dirserv_add_multiple_descriptors(const char *desc, uint8_t purpose,
         msg_out = NULL;
 
         r_tmp = dirserv_add_extrainfo(ei, &msg_out);
-        if (r_tmp < r) {
+        if (WRA_MORE_SEVERE(r_tmp, r)) {
           r = r_tmp;
           *msg = msg_out;
         }
@@ -638,25 +651,27 @@ dirserv_add_multiple_descriptors(const char *desc, uint8_t purpose,
     }
   }
 
-  return r <= 2 ? r : 2;
+  return r;
 }
 
 /** Examine the parsed server descriptor in <b>ri</b> and maybe insert it into
  * the list of server descriptors. Set *<b>msg</b> to a message that should be
  * passed back to the origin of this descriptor.
  *
+
  * Return 2 if descriptor is well-formed and accepted;
  *  1 if well-formed and accepted but origin should hear *msg;
  *  0 if well-formed but redundant with one we already have;
  * -1 if it is rejected and origin should hear *msg;
  *
+
  * This function is only called when fresh descriptors are posted, not when
  * we re-load the cache.
  */
-int
+was_router_added_t
 dirserv_add_descriptor(routerinfo_t *ri, const char **msg)
 {
-  int r;
+  was_router_added_t r;
   routerinfo_t *ri_old;
   char *desc = NULL;
   size_t desclen = 0;
@@ -703,11 +718,12 @@ dirserv_add_descriptor(routerinfo_t *ri, const char **msg)
     desc = tor_strndup(ri->cache_info.signed_descriptor_body, desclen);
   }
 
-  if ((r = router_add_to_routerlist(ri, msg, 0, 0))<0) {
+  r = router_add_to_routerlist(ri, msg, 0, 0);
-    if (r < -1 && desc) /* unless the routerinfo was fine, just out-of-date */
+  if (!WRA_WAS_ADDED(r)) {
+    /* unless the routerinfo was fine, just out-of-date */
+    if (WRA_WAS_REJECTED(r) && desc)
       control_event_or_authdir_new_descriptor("REJECTED", desc, desclen, *msg);
     tor_free(desc);
-    return r == -1 ? 0 : -1;
   } else {
     smartlist_t *changed;
     control_event_or_authdir_new_descriptor("ACCEPTED", desc, desclen, *msg);
@@ -721,8 +737,8 @@ dirserv_add_descriptor(routerinfo_t *ri, const char **msg)
         "Descriptor for invalid server accepted";
     }
     tor_free(desc);
-    return r == 0 ? 2 : 1;
   }
+  return r;
 }
 
 /** As dirserv_add_descriptor, but for an extrainfo_t <b>ei</b>. */

src/or/or.h

@@ -3410,10 +3410,12 @@ int dirserv_add_own_fingerprint(const char *nickname, crypto_pk_env_t *pk);
 int dirserv_load_fingerprint_file(void);
 void dirserv_free_fingerprint_list(void);
 const char *dirserv_get_nickname_by_digest(const char *digest);
-int dirserv_add_multiple_descriptors(const char *desc, uint8_t purpose,
+enum was_router_added_t dirserv_add_multiple_descriptors(
+                                     const char *desc, uint8_t purpose,
                                      const char *source,
                                      const char **msg);
-int dirserv_add_descriptor(routerinfo_t *ri, const char **msg);
+enum was_router_added_t dirserv_add_descriptor(routerinfo_t *ri,
+                                               const char **msg);
 int getinfo_helper_dirserv_unregistered(control_connection_t *conn,
                                         const char *question, char **answer);
 void dirserv_free_descriptors(void);
@@ -4388,8 +4390,41 @@ void routerlist_remove(routerlist_t *rl, routerinfo_t *ri, int make_old);
 void routerlist_free_all(void);
 void routerlist_reset_warnings(void);
 void router_set_status(const char *digest, int up);
-int router_add_to_routerlist(routerinfo_t *router, const char **msg,
+
-                             int from_cache, int from_fetch);
+/** Return value for router_add_to_routerlist() and dirserv_add_descriptor() */
+typedef enum was_router_added_t {
+  ROUTER_ADDED_SUCCESSFULLY = 0,
+  ROUTER_ADDED_NOTIFY_GENERATOR = 1,
+  ROUTER_WAS_NOT_NEW = -1,
+  ROUTER_NOT_IN_CONSENSUS = -2,
+  ROUTER_NOT_IN_CONSENSUS_OR_NETWORKSTATUS = -3,
+  ROUTER_AUTHDIR_REJECTS = -4,
+} was_router_added_t;
+
+static int WRA_WAS_ADDED(was_router_added_t s);
+static int WRA_WAS_OUTDATED(was_router_added_t s);
+static int WRA_WAS_REJECTED(was_router_added_t s);
+/**DOCDOC*/
+static INLINE int
+WRA_WAS_ADDED(was_router_added_t s) {
+  return s == ROUTER_ADDED_SUCCESSFULLY || s == ROUTER_ADDED_NOTIFY_GENERATOR;
+}
+/**DOCDOC*/
+static INLINE int WRA_WAS_OUTDATED(was_router_added_t s)
+{
+  return (s == ROUTER_WAS_NOT_NEW ||
+          s == ROUTER_NOT_IN_CONSENSUS ||
+          s == ROUTER_NOT_IN_CONSENSUS_OR_NETWORKSTATUS);
+}
+/**DOCDOC*/
+static INLINE int WRA_WAS_REJECTED(was_router_added_t s)
+{
+  return (s == ROUTER_AUTHDIR_REJECTS);
+}
+was_router_added_t router_add_to_routerlist(routerinfo_t *router,
+                                            const char **msg,
+                                            int from_cache,
+                                            int from_fetch);
 int router_add_extrainfo_to_routerlist(extrainfo_t *ei, const char **msg,
                                         int from_cache, int from_fetch);
 void routerlist_remove_old_routers(void);

src/or/router.c

@@ -541,7 +541,7 @@ init_keys(void)
         log_err(LD_GENERAL,"Generated a routerinfo we couldn't parse.");
         return -1;
       }
-      if (dirserv_add_descriptor(ri, &m) < 0) {
+      if (!WRA_WAS_ADDED(dirserv_add_descriptor(ri, &m))) {
         log_err(LD_GENERAL,"Unable to add own descriptor to directory: %s",
                 m?m:"<unknown error>");
         return -1;

src/or/routerlist.c

@@ -2923,7 +2923,7 @@ router_set_status(const char *digest, int up)
  * routers_update_status_from_consensus_networkstatus; subsequently, you
  * should call router_rebuild_store and routerlist_descriptors_added.
  */
-int
+was_router_added_t
 router_add_to_routerlist(routerinfo_t *router, const char **msg,
                          int from_cache, int from_fetch)
 {
@@ -2950,7 +2950,7 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg,
              router->nickname);
     *msg = "Router descriptor was not new.";
     routerinfo_free(router);
-    return -1;
+    return ROUTER_WAS_NOT_NEW;
   }
 
   if (authdir) {
@@ -2958,7 +2958,7 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg,
                                        !from_cache && !from_fetch)) {
       tor_assert(*msg);
       routerinfo_free(router);
-      return -2;
+      return ROUTER_AUTHDIR_REJECTS;
     }
     authdir_believes_valid = router->is_valid;
   } else if (from_fetch) {
@@ -2979,7 +2979,7 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg,
         signed_desc_append_to_journal(&router->cache_info,
                                       &routerlist->desc_store);
       routerlist_insert_old(routerlist, router);
-      return -1;
+      return ROUTER_NOT_IN_CONSENSUS_OR_NETWORKSTATUS;
     }
   }
 
@@ -3013,7 +3013,7 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg,
                                     &routerlist->desc_store);
     routerlist_insert_old(routerlist, router);
     *msg = "Skipping router descriptor: not in consensus.";
-    return -1;
+    return ROUTER_NOT_IN_CONSENSUS;
   }
 
   /* If we have a router with the same identity key, choose the newer one. */
@@ -3031,7 +3031,7 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg,
                                       &routerlist->desc_store);
       routerlist_insert_old(routerlist, router);
       *msg = "Router descriptor was not new.";
-      return -1;
+      return ROUTER_WAS_NOT_NEW;
     } else {
       /* Same key, and either new, or listed in the consensus. */
       log_debug(LD_DIR, "Replacing entry for router '%s/%s' [%s]",
@@ -3052,7 +3052,7 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg,
       *msg = authdir_believes_valid ? "Valid server updated" :
         ("Invalid server updated. (This dirserver is marking your "
          "server as unapproved.)");
-      return 0;
+      return ROUTER_ADDED_SUCCESSFULLY;
     }
   }
 
@@ -3063,7 +3063,7 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg,
     signed_desc_append_to_journal(&router->cache_info,
                                   &routerlist->desc_store);
   directory_set_dirty();
-  return 0;
+  return ROUTER_ADDED_SUCCESSFULLY;
 }
 
 /** Insert <b>ei</b> into the routerlist, or free it. Other arguments are
@@ -3390,7 +3390,7 @@ router_load_single_router(const char *s, uint8_t purpose, int cache,
                           const char **msg)
 {
   routerinfo_t *ri;
-  int r;
+  was_router_added_t r;
   smartlist_t *lst;
   char annotation_buf[ROUTER_ANNOTATION_BUF_LEN];
   tor_assert(msg);
@@ -3420,10 +3420,11 @@ router_load_single_router(const char *s, uint8_t purpose, int cache,
   smartlist_add(lst, ri);
   routers_update_status_from_consensus_networkstatus(lst, 0);
 
-  if ((r=router_add_to_routerlist(ri, msg, 0, 0))<0) {
+  r = router_add_to_routerlist(ri, msg, 0, 0);
+  if (!WRA_WAS_ADDED(r)) {
     /* we've already assigned to *msg now, and ri is already freed */
     tor_assert(*msg);
-    if (r < -1)
+    if (r == ROUTER_AUTHDIR_REJECTS)
       log_warn(LD_DIR, "Couldn't add router to list: %s Dropping.", *msg);
     smartlist_free(lst);
     return 0;
@@ -3469,8 +3470,8 @@ router_load_routers_from_string(const char *s, const char *eos,
 
   log_info(LD_DIR, "%d elements to add", smartlist_len(routers));
 
-  SMARTLIST_FOREACH(routers, routerinfo_t *, ri,
+  SMARTLIST_FOREACH_BEGIN(routers, routerinfo_t *, ri) {
-  {
+    was_router_added_t r;
     if (requested_fingerprints) {
       base16_encode(fp, sizeof(fp), descriptor_digests ?
                       ri->cache_info.signed_descriptor_digest :
@@ -3491,13 +3492,14 @@ router_load_routers_from_string(const char *s, const char *eos,
       }
     }
 
-    if (router_add_to_routerlist(ri, &msg, from_cache, !from_cache) >= 0) {
+    r = router_add_to_routerlist(ri, &msg, from_cache, !from_cache);
+    if (WRA_WAS_ADDED(r)) {
       any_changed = 1;
       smartlist_add(changed, ri);
       routerlist_descriptors_added(changed, from_cache);
       smartlist_clear(changed);
     }
-  });
+  } SMARTLIST_FOREACH_END(ri);
 
   routerlist_assert_ok(routerlist);
 
@@ -4182,14 +4184,14 @@ update_consensus_router_descriptor_downloads(time_t now)
              smartlist_len(no_longer_old));
     SMARTLIST_FOREACH(no_longer_old, signed_descriptor_t *, sd, {
         const char *msg;
-        int r;
+        was_router_added_t r;
         routerinfo_t *ri = routerlist_reparse_old(rl, sd);
         if (!ri) {
           log_warn(LD_BUG, "Failed to re-parse a router.");
           continue;
         }
         r = router_add_to_routerlist(ri, &msg, 1, 0);
-        if (r == -1) {
+        if (WRA_WAS_OUTDATED(r)) {
           log_warn(LD_DIR, "Couldn't add re-parsed router: %s",
                    msg?msg:"???");
         }

src/or/test.c

@@ -2862,9 +2862,9 @@ test_dir_format(void)
   r1->cache_info.published_on = time(NULL);
   r2->cache_info.published_on = time(NULL)-3*60*60;
   test_assert(router_dump_router_to_string(buf, 2048, r1, pk2)>0);
-  test_eq(dirserv_add_descriptor(buf,&m), 2);
+  test_eq(dirserv_add_descriptor(buf,&m), ROUTER_ADDED_NOTIFY_GENERATOR);
   test_assert(router_dump_router_to_string(buf, 2048, r2, pk1)>0);
-  test_eq(dirserv_add_descriptor(buf,&m), 2);
+  test_eq(dirserv_add_descriptor(buf,&m), ROUTER_ADDED_NOTIFY_GENERATOR);
   get_options()->Nickname = tor_strdup("DirServer");
   test_assert(!dirserv_dump_directory_to_string(&cp,pk3, 0));
   crypto_pk_get_digest(pk3, d);