Refactoring to make parse_bridge_line() unittestable.

- Make parse_bridge_line() return a struct.
- Make bridge_add_from_config() accept a struct.
- Make string_is_key_value() less hysterical.

4 files changed, 115 insertions, 96 deletions

diff --git a/src/or/config.c b/src/or/config.c
index a09dda996..9d0d56436 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -481,7 +481,6 @@ static int options_transition_affects_descriptor(
       const or_options_t *old_options, const or_options_t *new_options);
 static int check_nickname_list(const char *lst, const char *name, char **msg);
 
-static int parse_bridge_line(const char *line, int validate_only);
 static int parse_client_transport_line(const char *line, int validate_only);
 
 static int parse_server_transport_line(const char *line, int validate_only);
@@ -1293,11 +1292,13 @@ options_act(const or_options_t *old_options)
   if (options->Bridges) {
     mark_bridge_list();
     for (cl = options->Bridges; cl; cl = cl->next) {
-      if (parse_bridge_line(cl->value, 0)<0) {
+      bridge_line_t *bridge_line = parse_bridge_line(cl->value);
+      if (!bridge_line) {
         log_warn(LD_BUG,
                  "Previously validated Bridge line could not be added!");
         return -1;
       }
+      bridge_add_from_config(bridge_line);
     }
     sweep_bridge_list();
   }
@@ -2966,8 +2967,10 @@ options_validate(or_options_t *old_options, or_options_t *options,
     REJECT("If you set UseBridges, you must set TunnelDirConns.");
 
   for (cl = options->Bridges; cl; cl = cl->next) {
-    if (parse_bridge_line(cl->value, 1)<0)
-      REJECT("Bridge line did not parse. See logs for details.");
+      bridge_line_t *bridge_line = parse_bridge_line(cl->value);
+      if (!bridge_line)
+        REJECT("Bridge line did not parse. See logs for details.");
+      bridge_line_free(bridge_line);
   }
 
   for (cl = options->ClientTransportPlugin; cl; cl = cl->next) {
@@ -4038,8 +4041,10 @@ validate_transport_socks_arguments(const smartlist_t *args)
   tor_assert(smartlist_len(args) > 0);
 
   SMARTLIST_FOREACH_BEGIN(args, const char *, s) {
-    if (!string_is_key_value(s)) /* arguments should  be k=v items */
+    if (!string_is_key_value(LOG_WARN, s)) { /* items should be k=v items */
+      log_warn(LD_CONFIG, "'%s' is not a k=v item.", s);
       return -1;
+    }
   } SMARTLIST_FOREACH_END(s);
 
   socks_string = pt_stringify_socks_args(args);
@@ -4059,22 +4064,36 @@ validate_transport_socks_arguments(const smartlist_t *args)
   return 0;
 }
 
+/** Deallocate a bridge_line_t structure. */
+/* private */ void
+bridge_line_free(bridge_line_t *bridge_line)
+{
+  if (!bridge_line)
+    return;
+
+  if (bridge_line->socks_args) {
+    SMARTLIST_FOREACH(bridge_line->socks_args, char*, s, tor_free(s));
+    smartlist_free(bridge_line->socks_args);
+  }
+  tor_free(bridge_line->transport_name);
+  tor_free(bridge_line);
+}
+
 /** Read the contents of a Bridge line from <b>line</b>. Return 0
  * if the line is well-formed, and -1 if it isn't. If
  * <b>validate_only</b> is 0, and the line is well-formed, then add
- * the bridge described in the line to our internal bridge list. */
-static int
-parse_bridge_line(const char *line, int validate_only)
+ * the bridge described in the line to our internal bridge list.
+ *
+ * Bridge line format:
+ * Bridge [transport] IP:PORT [id-fingerprint] [k=v] [k=v] ...
+ */
+/* private */ bridge_line_t *
+parse_bridge_line(const char *line)
 {
   smartlist_t *items = NULL;
-  int r;
   char *addrport=NULL, *fingerprint=NULL;
-  char *transport_name=NULL;
   char *field=NULL;
-  tor_addr_t addr;
-  uint16_t port = 0;
-  char digest[DIGEST_LEN];
-  smartlist_t *socks_args = NULL;
+  bridge_line_t *bridge_line = tor_malloc_zero(sizeof(bridge_line_t));
 
   items = smartlist_new();
   smartlist_split_string(items, line, NULL,
@@ -4084,47 +4103,49 @@ parse_bridge_line(const char *line, int validate_only)
     goto err;
   }
 
-  /* field is either a transport name or addrport */
+  /* first field is either a transport name or addrport */
   field = smartlist_get(items, 0);
   smartlist_del_keeporder(items, 0);
 
-  if (!(strstr(field, ".") || strstr(field, ":"))) {
-    /* new-style bridge line */
-    transport_name = field;
+  if (string_is_C_identifier(field)) {
+    /* It's a transport name. */
+    bridge_line->transport_name = field;
     if (smartlist_len(items) < 1) {
       log_warn(LD_CONFIG, "Too few items to Bridge line.");
       goto err;
     }
-    addrport = smartlist_get(items, 0);
+    addrport = smartlist_get(items, 0); /* Next field is addrport then. */
     smartlist_del_keeporder(items, 0);
   } else {
     addrport = field;
   }
 
-  if (tor_addr_port_lookup(addrport, &addr, &port)<0) {
+  /* Parse addrport. */
+  if (tor_addr_port_lookup(addrport,
+                           &bridge_line->addr, &bridge_line->port)<0) {
     log_warn(LD_CONFIG, "Error parsing Bridge address '%s'", addrport);
     goto err;
   }
-  if (!port) {
+  if (!bridge_line->port) {
     log_info(LD_CONFIG,
              "Bridge address '%s' has no port; using default port 443.",
              addrport);
-    port = 443;
+    bridge_line->port = 443;
   }
 
   /* If transports are enabled, next field could be a fingerprint or a
-     socks argument. If transports are disabled, next field should be
+     socks argument. If transports are disabled, next field must be
      a fingerprint. */
   if (smartlist_len(items)) {
-    if (transport_name) { /* transports enabled: */
+    if (bridge_line->transport_name) { /* transports enabled: */
       field = smartlist_get(items, 0);
       smartlist_del_keeporder(items, 0);
 
       /* If it's a key=value pair, then it's a SOCKS argument for the
          transport proxy... */
-      if (string_is_key_value(field)) {
-        socks_args = smartlist_new();
-        smartlist_add(socks_args, field);
+      if (string_is_key_value(LOG_DEBUG, field)) {
+        bridge_line->socks_args = smartlist_new();
+        smartlist_add(bridge_line->socks_args, field);
       } else { /* ...otherwise, it's the bridge fingerprint. */
         fingerprint = field;
       }
@@ -4134,78 +4155,50 @@ parse_bridge_line(const char *line, int validate_only)
     }
   }
 
+  /* Handle fingerprint, if it was provided. */
   if (fingerprint) {
     if (strlen(fingerprint) != HEX_DIGEST_LEN) {
       log_warn(LD_CONFIG, "Key digest for Bridge is wrong length.");
       goto err;
     }
-    if (base16_decode(digest, DIGEST_LEN, fingerprint, HEX_DIGEST_LEN)<0) {
+    if (base16_decode(bridge_line->digest, DIGEST_LEN,
+                      fingerprint, HEX_DIGEST_LEN)<0) {
       log_warn(LD_CONFIG, "Unable to decode Bridge key digest.");
       goto err;
     }
   }
 
   /* If we are using transports, any remaining items in the smartlist
-     must be k=v values. */
-  if (transport_name && smartlist_len(items)) {
-    if (!socks_args)
-      socks_args = smartlist_new();
+     should be k=v values. */
+  if (bridge_line->transport_name && smartlist_len(items)) {
+    if (!bridge_line->socks_args)
+      bridge_line->socks_args = smartlist_new();
 
     /* append remaining items of 'items' to 'socks_args' */
-    smartlist_add_all(socks_args, items);
+    smartlist_add_all(bridge_line->socks_args, items);
     smartlist_clear(items);
 
-    tor_assert(smartlist_len(socks_args) > 0);
+    tor_assert(smartlist_len(bridge_line->socks_args) > 0);
   }
 
-  if (!validate_only) {
-    log_debug(LD_DIR, "Bridge at %s (transport: %s) (%s)",
-              fmt_addrport(&addr, port),
-              transport_name ? transport_name : "no transport",
-              fingerprint ? fingerprint : "no key listed");
-
-    if (socks_args) { /* print socks arguments */
-      int i = 0;
-
-      tor_assert(smartlist_len(socks_args) > 0);
-
-      log_debug(LD_DIR, "Bridge uses %d SOCKS arguments:",
-                smartlist_len(socks_args));
-      SMARTLIST_FOREACH(socks_args, const char *, arg,
-                        log_debug(LD_CONFIG, "%d: %s", ++i, arg));
-    }
-
-    bridge_add_from_config(&addr, port,
-                           fingerprint ? digest : NULL,
-                           transport_name, socks_args);
-  } else {
-    if (socks_args) {
-      if (validate_transport_socks_arguments(socks_args) < 0)
-        goto err;
-    }
+  if (bridge_line->socks_args) {
+    if (validate_transport_socks_arguments(bridge_line->socks_args) < 0)
+      goto err;
   }
 
-  r = 0;
   goto done;
 
  err:
-  r = -1;
+  bridge_line_free(bridge_line);
+  bridge_line = NULL;
 
  done:
   SMARTLIST_FOREACH(items, char*, s, tor_free(s));
   smartlist_free(items);
   tor_free(addrport);
-  tor_free(transport_name);
   tor_free(fingerprint);
 
-  /* We only have to free socks_args if we are validating, since
-     otherwise bridge_add_from_config() steals its reference. */
-  if (socks_args && validate_only) {
-    SMARTLIST_FOREACH(socks_args, char*, s, tor_free(s));
-    smartlist_free(socks_args);
-  }
-
-  return r;
+  return bridge_line;
 }
 
 /** Read the contents of a ClientTransportPlugin line from
diff --git a/src/or/config.h b/src/or/config.h
index 8e3465580..b5c0c734b 100644
--- a/src/or/config.h
+++ b/src/or/config.h
@@ -96,5 +96,19 @@ int addressmap_register_auto(const char *from, const char *to,
                              addressmap_entry_source_t addrmap_source,
                              const char **msg);
 
+/** Represents the information stored in a torrc Bridge line. */
+typedef struct bridge_line_t {
+  tor_addr_t addr; /* The IP address of the bridge. */
+  uint16_t port; /* The TCP port of the bridge. */
+  char *transport_name; /* The name of the pluggable transport that
+                           should be used to connect to the bridge. */
+  char digest[DIGEST_LEN]; /* The bridge's identity key digest. */
+  smartlist_t *socks_args;; /* SOCKS arguments for the pluggable
+                               transport proxy. */
+} bridge_line_t;
+
+void bridge_line_free(bridge_line_t *bridge_line);
+bridge_line_t *parse_bridge_line(const char *line);
+
 #endif
 
diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c
index a07670bbd..44041d35d 100644
--- a/src/or/entrynodes.c
+++ b/src/or/entrynodes.c
@@ -1633,37 +1633,52 @@ bridge_resolve_conflicts(const tor_addr_t *addr, uint16_t port,
   } SMARTLIST_FOREACH_END(bridge);
 }
 
-/** Remember a new bridge at <b>addr</b>:<b>port</b>. If <b>digest</b>
- * is set, it tells us the identity key too.  If we already had the
- * bridge in our list, unmark it, and don't actually add anything new.
- * If <b>transport_name</b> is non-NULL - the bridge is associated with a
- * pluggable transport - we assign the transport to the bridge.
- * If <b>transport_name</b> is non-NULL - the bridge is associated
- * with a pluggable transport - we assign the transport to the bridge.
- * If <b>socks_args</b> is non-NULL, it's a smartlist carrying
- * key=value pairs to be passed to the pluggable transports
- * proxy. This function steals reference of the smartlist. */
+/** Register the bridge information in <b>bridge_line</b> to the
+ *  bridge subsystem. Steals reference of <b>bridge_line</b>. */
 void
-bridge_add_from_config(const tor_addr_t *addr, uint16_t port,
-                       const char *digest, const char *transport_name,
-                       smartlist_t *socks_args)
+bridge_add_from_config(bridge_line_t *bridge_line)
 {
   bridge_info_t *b;
 
-  bridge_resolve_conflicts(addr, port, digest, transport_name);
+  { /* Log the bridge we are about to register: */
+    log_debug(LD_GENERAL, "Registering bridge at %s (transport: %s) (%s)",
+              fmt_addrport(&bridge_line->addr, bridge_line->port),
+              bridge_line->transport_name ?
+              bridge_line->transport_name : "no transport",
+              tor_digest_is_zero(bridge_line->digest) ?
+              "no key listed" : hex_str(bridge_line->digest, DIGEST_LEN));
+
+    if (bridge_line->socks_args) { /* print socks arguments */
+      int i = 0;
+
+      tor_assert(smartlist_len(bridge_line->socks_args) > 0);
+
+      log_debug(LD_GENERAL, "Bridge uses %d SOCKS arguments:",
+                smartlist_len(bridge_line->socks_args));
+      SMARTLIST_FOREACH(bridge_line->socks_args, const char *, arg,
+                        log_debug(LD_CONFIG, "%d: %s", ++i, arg));
+    }
+  }
+
+  bridge_resolve_conflicts(&bridge_line->addr,
+                           bridge_line->port,
+                           bridge_line->digest,
+                           bridge_line->transport_name);
 
   b = tor_malloc_zero(sizeof(bridge_info_t));
-  tor_addr_copy(&b->addr, addr);
-  b->port = port;
-  if (digest)
-    memcpy(b->identity, digest, DIGEST_LEN);
-  if (transport_name)
-    b->transport_name = tor_strdup(transport_name);
+  tor_addr_copy(&b->addr, &bridge_line->addr);
+  b->port = bridge_line->port;
+  if (bridge_line->digest)
+    memcpy(b->identity, bridge_line->digest, DIGEST_LEN);
+  if (bridge_line->transport_name)
+    b->transport_name = bridge_line->transport_name;
   b->fetch_status.schedule = DL_SCHED_BRIDGE;
-  b->socks_args = socks_args;
+  b->socks_args = bridge_line->socks_args;
   if (!bridge_list)
     bridge_list = smartlist_new();
 
+  tor_free(bridge_line); /* Deallocate bridge_line now. */
+
   smartlist_add(bridge_list, b);
 }
 
diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h
index 48f678a18..6a4bcea48 100644
--- a/src/or/entrynodes.h
+++ b/src/or/entrynodes.h
@@ -97,11 +97,8 @@ int routerinfo_is_a_configured_bridge(const routerinfo_t *ri);
 int node_is_a_configured_bridge(const node_t *node);
 void learned_router_identity(const tor_addr_t *addr, uint16_t port,
                              const char *digest);
-struct smartlist_t;
-void bridge_add_from_config(const tor_addr_t *addr, uint16_t port,
-                            const char *digest,
-                            const char *transport_name,
-                            struct smartlist_t *socks_args);
+struct bridge_line_t;
+void bridge_add_from_config(struct bridge_line_t *bridge_line);
 void retry_bridge_descriptor_fetch_directly(const char *digest);
 void fetch_bridge_descriptors(const or_options_t *options, time_t now);
 void learned_bridge_descriptor(routerinfo_t *ri, int from_cache);