[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-commits] [onionoo/master] Avoid silently exiting methods.



commit 817a29c62670468170094f891c3ffc0c7c4c448e
Author: Karsten Loesing <karsten.loesing@xxxxxxx>
Date:   Mon Dec 8 11:22:32 2014 +0100

    Avoid silently exiting methods.
    
    Whenever we return from a method before it ends, we should either log a
    warning or leave a comment why this is intended behavior.
    
    Suggested by iwakeh on #12651.
---
 .../torproject/onionoo/docs/ClientsHistory.java    |   20 ++++++++++++++++++++
 .../org/torproject/onionoo/docs/DocumentStore.java |   16 ++++++++++++++++
 .../org/torproject/onionoo/docs/UptimeHistory.java |   18 ++++++++++++++++--
 .../org/torproject/onionoo/server/NodeIndexer.java |    1 +
 .../torproject/onionoo/server/RequestHandler.java  |   14 ++++++++++++++
 .../torproject/onionoo/server/ResourceServlet.java |   13 +++++++++++++
 .../onionoo/updater/ClientsStatusUpdater.java      |    3 +++
 .../onionoo/updater/DescriptorQueue.java           |    2 ++
 .../onionoo/updater/NodeDetailsStatusUpdater.java  |    2 ++
 .../onionoo/writer/ClientsDocumentWriter.java      |    3 +++
 .../onionoo/writer/UptimeDocumentWriter.java       |    4 ++++
 .../onionoo/writer/WeightsDocumentWriter.java      |    3 +++
 12 files changed, 97 insertions(+), 2 deletions(-)

diff --git a/src/main/java/org/torproject/onionoo/docs/ClientsHistory.java b/src/main/java/org/torproject/onionoo/docs/ClientsHistory.java
index 35a063a..effddf5 100644
--- a/src/main/java/org/torproject/onionoo/docs/ClientsHistory.java
+++ b/src/main/java/org/torproject/onionoo/docs/ClientsHistory.java
@@ -6,8 +6,14 @@ import java.util.Map;
 import java.util.SortedMap;
 import java.util.TreeMap;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 public class ClientsHistory implements Comparable<ClientsHistory> {
 
+  private final static Logger log = LoggerFactory.getLogger(
+      ClientsHistory.class);
+
   private long startMillis;
   public long getStartMillis() {
     return this.startMillis;
@@ -55,20 +61,29 @@ public class ClientsHistory implements Comparable<ClientsHistory> {
       String responseHistoryString) {
     String[] parts = responseHistoryString.split(" ", 8);
     if (parts.length != 8) {
+      log.warn("Invalid number of space-separated strings in clients "
+          + "history: '" + responseHistoryString + "'.  Skipping");
       return null;
     }
     long startMillis = DateTimeHelper.parse(parts[0] + " " + parts[1]);
     long endMillis = DateTimeHelper.parse(parts[2] + " " + parts[3]);
     if (startMillis < 0L || endMillis < 0L) {
+      log.warn("Invalid start or end timestamp in clients history: '"
+          + responseHistoryString + "'.  Skipping.");
       return null;
     }
     if (startMillis >= endMillis) {
+      log.warn("Start timestamp must be smaller than end timestamp in "
+          + "clients history: '" + responseHistoryString
+          + "'.  Skipping.");
       return null;
     }
     double totalResponses = 0.0;
     try {
       totalResponses = Double.parseDouble(parts[4]);
     } catch (NumberFormatException e) {
+      log.warn("Invalid response number format in clients history: '"
+          + responseHistoryString + "'.  Skipping.");
       return null;
     }
     SortedMap<String, Double> responsesByCountry =
@@ -79,6 +94,9 @@ public class ClientsHistory implements Comparable<ClientsHistory> {
         parseResponses(parts[7]);
     if (responsesByCountry == null || responsesByTransport == null ||
         responsesByVersion == null) {
+      log.warn("Invalid format of responses by country, transport, or "
+          + "version in clients history: '" + responseHistoryString
+          + "'.  Skipping.");
       return null;
     }
     return new ClientsHistory(startMillis, endMillis, totalResponses,
@@ -92,12 +110,14 @@ public class ClientsHistory implements Comparable<ClientsHistory> {
       for (String pair : responsesString.split(",")) {
         String[] keyValue = pair.split("=");
         if (keyValue.length != 2) {
+          /* Logged by caller */
           return null;
         }
         double value = 0.0;
         try {
           value = Double.parseDouble(keyValue[1]);
         } catch (NumberFormatException e) {
+          /* Logged by caller */
           return null;
         }
         responses.put(keyValue[0], value);
diff --git a/src/main/java/org/torproject/onionoo/docs/DocumentStore.java b/src/main/java/org/torproject/onionoo/docs/DocumentStore.java
index 4b7bca5..a880e53 100644
--- a/src/main/java/org/torproject/onionoo/docs/DocumentStore.java
+++ b/src/main/java/org/torproject/onionoo/docs/DocumentStore.java
@@ -393,6 +393,9 @@ public class DocumentStore {
     DetailsDocument detailsDocument = this.retrieveDocumentFile(
         DetailsDocument.class, true, fingerprint);
     if (detailsDocument == null) {
+      /* There is no details document available that we could serve as
+       * basis for generating a summary document on-the-fly.  Nothing to
+       * worry about. */
       return null;
     }
     boolean isRelay = detailsDocument.getHashedFingerprint() == null;
@@ -402,6 +405,10 @@ public class DocumentStore {
     String countryCode = null, aSNumber = null, contact = null;
     for (String orAddressAndPort : detailsDocument.getOrAddresses()) {
       if (!orAddressAndPort.contains(":")) {
+        log.warn("Attempt to create summary document from details "
+            + "document for fingerprint " + fingerprint + " failed "
+            + "because of invalid OR address/port: '" + orAddressAndPort
+            + "'.  Not returning a summary document in this case.");
         return null;
       }
       String orAddress = orAddressAndPort.substring(0,
@@ -431,6 +438,7 @@ public class DocumentStore {
       Class<T> documentType, boolean parse, String fingerprint) {
     File documentFile = this.getDocumentFile(documentType, fingerprint);
     if (documentFile == null || !documentFile.exists()) {
+      /* Document file does not exist.  That's okay. */
       return null;
     } else if (documentFile.isDirectory()) {
       log.error("Could not read file '"
@@ -451,6 +459,7 @@ public class DocumentStore {
       bis.close();
       byte[] allData = baos.toByteArray();
       if (allData.length == 0) {
+        /* Document file is empty. */
         return null;
       }
       documentString = new String(allData, "US-ASCII");
@@ -598,6 +607,9 @@ public class DocumentStore {
     File documentFile = null;
     if (fingerprint == null && !documentType.equals(UpdateStatus.class) &&
         !documentType.equals(UptimeStatus.class)) {
+      log.warn("Attempted to locate a document file of type "
+          + documentType.getName() + " without providing a fingerprint.  "
+          + "Such a file does not exist.");
       return null;
     }
     File directory = null;
@@ -685,6 +697,8 @@ public class DocumentStore {
   private void writeNodeStatuses() {
     File directory = this.statusDir;
     if (directory == null) {
+      log.error("Unable to write node statuses without knowing the "
+          + "'status' directory to write to!");
       return;
     }
     File summaryFile = new File(directory, "summary");
@@ -766,6 +780,8 @@ public class DocumentStore {
 
   private void writeUpdateStatus() {
     if (this.outDir == null) {
+      log.error("Unable to write update status file without knowing the "
+          + "'out' directory to write to!");
       return;
     }
     UpdateStatus updateStatus = new UpdateStatus();
diff --git a/src/main/java/org/torproject/onionoo/docs/UptimeHistory.java b/src/main/java/org/torproject/onionoo/docs/UptimeHistory.java
index 0d27242..b686c7e 100644
--- a/src/main/java/org/torproject/onionoo/docs/UptimeHistory.java
+++ b/src/main/java/org/torproject/onionoo/docs/UptimeHistory.java
@@ -1,8 +1,13 @@
 package org.torproject.onionoo.docs;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
-public class UptimeHistory
-    implements Comparable<UptimeHistory> {
+
+public class UptimeHistory implements Comparable<UptimeHistory> {
+
+  private final static Logger log = LoggerFactory.getLogger(
+      UptimeHistory.class);
 
   private boolean relay;
   public boolean isRelay() {
@@ -29,23 +34,32 @@ public class UptimeHistory
   public static UptimeHistory fromString(String uptimeHistoryString) {
     String[] parts = uptimeHistoryString.split(" ", 3);
     if (parts.length != 3) {
+      log.warn("Invalid number of space-separated strings in uptime "
+          + "history: '" + uptimeHistoryString + "'.  Skipping");
       return null;
     }
     boolean relay = false;
     if (parts[0].equals("r")) {
       relay = true;
     } else if (!parts[0].equals("b")) {
+      log.warn("Invalid node type in uptime history: '"
+          + uptimeHistoryString + "'.  Supported types are 'r' and 'b'.  "
+          + "Skipping.");
       return null;
     }
     long startMillis = DateTimeHelper.parse(parts[1],
           DateTimeHelper.DATEHOUR_NOSPACE_FORMAT);
     if (DateTimeHelper.NO_TIME_AVAILABLE == startMillis) {
+      log.warn("Invalid start timestamp in uptime history: '"
+          + uptimeHistoryString + "'.  Skipping.");
       return null;
     }
     int uptimeHours = -1;
     try {
       uptimeHours = Integer.parseInt(parts[2]);
     } catch (NumberFormatException e) {
+      log.warn("Invalid number format in uptime history: '"
+          + uptimeHistoryString + "'.  Skipping.");
       return null;
     }
     return new UptimeHistory(relay, startMillis, uptimeHours);
diff --git a/src/main/java/org/torproject/onionoo/server/NodeIndexer.java b/src/main/java/org/torproject/onionoo/server/NodeIndexer.java
index f8870db..1a67cc5 100644
--- a/src/main/java/org/torproject/onionoo/server/NodeIndexer.java
+++ b/src/main/java/org/torproject/onionoo/server/NodeIndexer.java
@@ -105,6 +105,7 @@ public class NodeIndexer implements ServletContextListener, Runnable {
     }
     synchronized (this) {
       if (updateStatusMillis <= this.lastIndexed) {
+        /* Index on disk is no more recent than the one in memory. */
         return;
       }
     }
diff --git a/src/main/java/org/torproject/onionoo/server/RequestHandler.java b/src/main/java/org/torproject/onionoo/server/RequestHandler.java
index 28b5531..2d8dd58 100644
--- a/src/main/java/org/torproject/onionoo/server/RequestHandler.java
+++ b/src/main/java/org/torproject/onionoo/server/RequestHandler.java
@@ -154,6 +154,7 @@ public class RequestHandler {
 
   private void filterByType() {
     if (this.type == null) {
+      /* Not filtering by type. */
       return;
     } else if (this.type.equals("relay")) {
       this.filteredBridges.clear();
@@ -164,6 +165,7 @@ public class RequestHandler {
 
   private void filterByRunning() {
     if (this.running == null) {
+      /* Not filtering by running or not. */
       return;
     }
     boolean runningRequested = this.running.equals("true");
@@ -191,6 +193,7 @@ public class RequestHandler {
 
   private void filterBySearchTerms() {
     if (this.search == null) {
+      /* Not filtering by search terms. */
       return;
     }
     for (String searchTerm : this.search) {
@@ -276,6 +279,7 @@ public class RequestHandler {
 
   private void filterByLookup() {
     if (this.lookup == null) {
+      /* Not filtering by looking up relay or bridge. */
       return;
     }
     String fingerprint = this.lookup;
@@ -293,6 +297,7 @@ public class RequestHandler {
 
   private void filterByFingerprint() {
     if (this.fingerprint == null) {
+      /* Not filtering by fingerprint. */
       return;
     }
     this.filteredRelays.clear();
@@ -311,6 +316,7 @@ public class RequestHandler {
 
   private void filterByCountryCode() {
     if (this.country == null) {
+      /* Not filtering by country code. */
       return;
     }
     String countryCode = this.country.toLowerCase();
@@ -335,6 +341,7 @@ public class RequestHandler {
 
   private void filterByASNumber() {
     if (this.as == null) {
+      /* Not filtering by AS number. */
       return;
     }
     String aSNumber = this.as.toUpperCase();
@@ -361,6 +368,7 @@ public class RequestHandler {
 
   private void filterByFlag() {
     if (this.flag == null) {
+      /* Not filtering by relay flag. */
       return;
     }
     String flag = this.flag.toLowerCase();
@@ -398,6 +406,7 @@ public class RequestHandler {
 
   private void filterNodesByFirstSeenDays() {
     if (this.firstSeenDays == null) {
+      /* Not filtering by first-seen days. */
       return;
     }
     filterNodesByDays(this.filteredRelays,
@@ -408,6 +417,7 @@ public class RequestHandler {
 
   private void filterNodesByLastSeenDays() {
     if (this.lastSeenDays == null) {
+      /* Not filtering by last-seen days. */
       return;
     }
     filterNodesByDays(this.filteredRelays,
@@ -436,6 +446,7 @@ public class RequestHandler {
 
   private void filterByContact() {
     if (this.contact == null) {
+      /* Not filtering by contact information. */
       return;
     }
     Set<String> removeRelays = new HashSet<String>();
@@ -458,6 +469,7 @@ public class RequestHandler {
 
   private void filterByFamily() {
     if (this.family == null) {
+      /* Not filtering by relay family. */
       return;
     }
     Set<String> removeRelays = new HashSet<String>(
@@ -506,6 +518,7 @@ public class RequestHandler {
 
   private void offset() {
     if (this.offset == null) {
+      /* Not skipping first results. */
       return;
     }
     int offsetValue = Integer.parseInt(this.offset);
@@ -522,6 +535,7 @@ public class RequestHandler {
 
   private void limit() {
     if (this.limit == null) {
+      /* Not limiting number of results. */
       return;
     }
     int limitValue = Integer.parseInt(this.limit);
diff --git a/src/main/java/org/torproject/onionoo/server/ResourceServlet.java b/src/main/java/org/torproject/onionoo/server/ResourceServlet.java
index f6f060a..6182704 100644
--- a/src/main/java/org/torproject/onionoo/server/ResourceServlet.java
+++ b/src/main/java/org/torproject/onionoo/server/ResourceServlet.java
@@ -372,6 +372,7 @@ public class ResourceServlet extends HttpServlet {
     Matcher searchQueryStringMatcher = searchQueryStringPattern.matcher(
         queryString);
     if (!searchQueryStringMatcher.matches()) {
+      /* Search query contains illegal character(s). */
       return null;
     }
     String parameter = searchQueryStringMatcher.group(1);
@@ -383,6 +384,7 @@ public class ResourceServlet extends HttpServlet {
     }
     for (String searchParameter : searchParameters) {
       if (!searchParameterPattern.matcher(searchParameter).matches()) {
+        /* Illegal search term. */
         return null;
       }
     }
@@ -393,9 +395,11 @@ public class ResourceServlet extends HttpServlet {
       Pattern.compile("^[0-9a-zA-Z]{1,40}$");
   private String parseFingerprintParameter(String parameter) {
     if (!fingerprintParameterPattern.matcher(parameter).matches()) {
+      /* Fingerprint contains non-hex character(s). */
       return null;
     }
     if (parameter.length() != 40) {
+      /* Only full fingerprints are accepted. */
       return null;
     }
     return parameter;
@@ -405,6 +409,8 @@ public class ResourceServlet extends HttpServlet {
       Pattern.compile("^[0-9a-zA-Z]{2}$");
   private String parseCountryCodeParameter(String parameter) {
     if (!countryCodeParameterPattern.matcher(parameter).matches()) {
+      /* Country code contains illegal characters or is shorter/longer
+       * than 2 characters. */
       return null;
     }
     return parameter;
@@ -414,6 +420,7 @@ public class ResourceServlet extends HttpServlet {
       Pattern.compile("^[asAS]{0,2}[0-9]{1,10}$");
   private String parseASNumberParameter(String parameter) {
     if (!aSNumberParameterPattern.matcher(parameter).matches()) {
+      /* AS number contains illegal character(s). */
       return null;
     }
     return parameter;
@@ -423,6 +430,7 @@ public class ResourceServlet extends HttpServlet {
       Pattern.compile("^[a-zA-Z0-9]{1,20}$");
   private String parseFlagParameter(String parameter) {
     if (!flagPattern.matcher(parameter).matches()) {
+      /* Flag contains illegal character(s). */
       return null;
     }
     return parameter;
@@ -431,6 +439,7 @@ public class ResourceServlet extends HttpServlet {
   private static Pattern daysPattern = Pattern.compile("^[0-9-]{1,10}$");
   private int[] parseDaysParameter(String parameter) {
     if (!daysPattern.matcher(parameter).matches()) {
+      /* Days contain illegal character(s). */
       return null;
     }
     int x = 0, y = Integer.MAX_VALUE;
@@ -448,9 +457,11 @@ public class ResourceServlet extends HttpServlet {
         }
       }
     } catch (NumberFormatException e) {
+      /* Invalid format. */
       return null;
     }
     if (x > y) {
+      /* Second number or days must exceed first number. */
       return null;
     }
     return new int[] { x, y };
@@ -459,6 +470,7 @@ public class ResourceServlet extends HttpServlet {
   private String[] parseContactParameter(String parameter) {
     for (char c : parameter.toCharArray()) {
       if (c < 32 || c >= 127) {
+        /* Only accept printable ASCII. */
         return null;
       }
     }
@@ -469,6 +481,7 @@ public class ResourceServlet extends HttpServlet {
       Pattern.compile("^[0-9a-zA-Z_,]*$");
   private String[] parseFieldsParameter(String parameter) {
     if (!fieldsParameterPattern.matcher(parameter).matches()) {
+      /* Fields contain illegal character(s). */
       return null;
     }
     return parameter.split(",");
diff --git a/src/main/java/org/torproject/onionoo/updater/ClientsStatusUpdater.java b/src/main/java/org/torproject/onionoo/updater/ClientsStatusUpdater.java
index f3f2e0f..72f5a92 100644
--- a/src/main/java/org/torproject/onionoo/updater/ClientsStatusUpdater.java
+++ b/src/main/java/org/torproject/onionoo/updater/ClientsStatusUpdater.java
@@ -73,10 +73,13 @@ public class ClientsStatusUpdater implements DescriptorListener,
     if (dirreqStatsEndMillis < 0L ||
         dirreqStatsIntervalLengthMillis != DateTimeHelper.ONE_DAY ||
         responses == null || !responses.containsKey("ok")) {
+      /* No directory request responses in the descriptor that we would
+       * include in a clients document. */
       return;
     }
     double okResponses = (double) (responses.get("ok") - 4);
     if (okResponses < 0.0) {
+      /* Response numbers are not supposed to be negative.  Skipping. */
       return;
     }
     String hashedFingerprint = descriptor.getFingerprint().toUpperCase();
diff --git a/src/main/java/org/torproject/onionoo/updater/DescriptorQueue.java b/src/main/java/org/torproject/onionoo/updater/DescriptorQueue.java
index 8ba4260..93a291c 100644
--- a/src/main/java/org/torproject/onionoo/updater/DescriptorQueue.java
+++ b/src/main/java/org/torproject/onionoo/updater/DescriptorQueue.java
@@ -168,6 +168,8 @@ class DescriptorQueue {
 
   public void writeHistoryFile() {
     if (this.historyFile == null) {
+      log.error("Unable to write history file, possibly because of an "
+          + "unknown descriptor type.  Skipping.");
       return;
     }
     SortedMap<String, Long> excludedAndParsedFiles =
diff --git a/src/main/java/org/torproject/onionoo/updater/NodeDetailsStatusUpdater.java b/src/main/java/org/torproject/onionoo/updater/NodeDetailsStatusUpdater.java
index 935aa4e..ef87233 100644
--- a/src/main/java/org/torproject/onionoo/updater/NodeDetailsStatusUpdater.java
+++ b/src/main/java/org/torproject/onionoo/updater/NodeDetailsStatusUpdater.java
@@ -150,6 +150,7 @@ public class NodeDetailsStatusUpdater implements DescriptorListener,
     } else if (detailsStatus.getDescPublished() != null &&
         detailsStatus.getDescPublished() >=
         descriptor.getPublishedMillis()) {
+      /* Already parsed more recent server descriptor from this relay. */
       return;
     }
     long lastRestartedMillis = descriptor.getPublishedMillis()
@@ -276,6 +277,7 @@ public class NodeDetailsStatusUpdater implements DescriptorListener,
       detailsStatus = new DetailsStatus();
     } else if (descriptor.getPublishedMillis() <=
         detailsStatus.getDescPublished()) {
+      /* Already parsed more recent server descriptor from this bridge. */
       return;
     }
     long lastRestartedMillis = descriptor.getPublishedMillis()
diff --git a/src/main/java/org/torproject/onionoo/writer/ClientsDocumentWriter.java b/src/main/java/org/torproject/onionoo/writer/ClientsDocumentWriter.java
index 885494c..84d5a30 100644
--- a/src/main/java/org/torproject/onionoo/writer/ClientsDocumentWriter.java
+++ b/src/main/java/org/torproject/onionoo/writer/ClientsDocumentWriter.java
@@ -198,6 +198,7 @@ public class ClientsDocumentWriter implements DocumentWriter {
       }
     }
     if (firstNonNullIndex < 0) {
+      /* Not a single non-negative value in the data points. */
       return null;
     }
     long firstDataPointMillis = (((this.now - graphInterval)
@@ -273,6 +274,8 @@ public class ClientsDocumentWriter implements DocumentWriter {
     if (foundTwoAdjacentDataPoints) {
       return graphHistory;
     } else {
+      /* There are no two adjacent values in the data points that are
+       * required to draw a line graph. */
       return null;
     }
   }
diff --git a/src/main/java/org/torproject/onionoo/writer/UptimeDocumentWriter.java b/src/main/java/org/torproject/onionoo/writer/UptimeDocumentWriter.java
index 093bc98..c5f71a1 100644
--- a/src/main/java/org/torproject/onionoo/writer/UptimeDocumentWriter.java
+++ b/src/main/java/org/torproject/onionoo/writer/UptimeDocumentWriter.java
@@ -39,6 +39,7 @@ public class UptimeDocumentWriter implements DocumentWriter {
     UptimeStatus uptimeStatus = this.documentStore.retrieve(
         UptimeStatus.class, true);
     if (uptimeStatus == null) {
+      /* No global uptime information available. */
       return;
     }
     UpdateStatus updateStatus = this.documentStore.retrieve(
@@ -233,6 +234,7 @@ public class UptimeDocumentWriter implements DocumentWriter {
       }
     }
     if (firstNonNullIndex < 0) {
+      /* Not a single non-negative value in the data points. */
       return null;
     }
     long firstDataPointMillis = (((this.now - graphInterval)
@@ -273,6 +275,8 @@ public class UptimeDocumentWriter implements DocumentWriter {
     if (foundTwoAdjacentDataPoints) {
       return graphHistory;
     } else {
+      /* There are no two adjacent values in the data points that are
+       * required to draw a line graph. */
       return null;
     }
   }
diff --git a/src/main/java/org/torproject/onionoo/writer/WeightsDocumentWriter.java b/src/main/java/org/torproject/onionoo/writer/WeightsDocumentWriter.java
index 7d52a47..62ff533 100644
--- a/src/main/java/org/torproject/onionoo/writer/WeightsDocumentWriter.java
+++ b/src/main/java/org/torproject/onionoo/writer/WeightsDocumentWriter.java
@@ -157,6 +157,7 @@ public class WeightsDocumentWriter implements DocumentWriter {
       }
     }
     if (firstNonNullIndex < 0) {
+      /* Not a single non-negative value in the data points. */
       return null;
     }
     long firstDataPointMillis = (((this.now - graphInterval)
@@ -199,6 +200,8 @@ public class WeightsDocumentWriter implements DocumentWriter {
     if (foundTwoAdjacentDataPoints) {
       return graphHistory;
     } else {
+      /* There are no two adjacent values in the data points that are
+       * required to draw a line graph. */
       return null;
     }
   }

_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits