Better determination of Symmetric NAT

webrtc/p2p/stunprober/main.cc

 /*
  *  Copyright 2015 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 20 matching lines @@
 using stunprober::HostNameResolverInterface;
 using stunprober::TaskRunner;
 using stunprober::SocketFactory;
 using stunprober::StunProber;
 using stunprober::AsyncCallback;
 using stunprober::ClientSocketInterface;
 using stunprober::ServerSocketInterface;
 using stunprober::SocketFactory;
 using stunprober::TaskRunner;
 
+using NATTYPE = stunprober::StunProber::NATTYPE;
+
 DEFINE_bool(help, false, "Prints this message");
 DEFINE_int(interval, 10, "Interval of consecutive stun pings in milliseconds");
 DEFINE_bool(shared_socket, false, "Share socket mode for different remote IPs");
 DEFINE_int(pings_per_ip,
            10,
            "Number of consecutive stun pings to send for each IP");
 DEFINE_int(timeout,
            1000,
            "Milliseconds of wait after the last ping sent before exiting");
 DEFINE_string(
@@ skipping 60 matching lines @@
                           int interval_ms,
                           std::string suffix) {
   char output[1000];
   rtc::sprintfn(output, sizeof(output), "NetConnectivity6.%s.%s.%dms.%s",
                 behind_nat ? "NAT" : "NoNAT",
                 is_src_port_shared ? "SrcPortShared" : "SrcPortUnique",
                 interval_ms, suffix.c_str());
   return std::string(output);
 }
 
+const char* PrintNATType(NATTYPE type) {
+  switch (type) {
+    case NATTYPE::NATTYPE_NO_NAT:

[pthatcher2, 2015/06/05 22:39:41]

Do you really need "NATTYPE::" Prefixed everywhere?  I don't think you do.

[pthatcher2, 2015/06/05 22:39:41]

NATTYPE_NONE?

[guoweis_webrtc, 2015/06/07 17:29:39]

Done.

[guoweis_webrtc, 2015/06/07 17:29:39]

Done.

+      return "not behind NAT";
+    case NATTYPE::NATTYPE_NAT_TYPE_UNKNOWN:

[pthatcher2, 2015/06/05 22:39:41]

Why not NATTYPE_UNKNOWN?

[guoweis_webrtc, 2015/06/07 17:29:39]

Done.

+      return "unknown NAT type";
+    case NATTYPE::NATTYPE_SYM_NAT:

[pthatcher2, 2015/06/05 22:39:41]

NATTYPE_SYMMETRIC?

[guoweis_webrtc, 2015/06/07 17:29:39]

Done.

+      return "Symmetric NAT";
+    case NATTYPE::NATTYPE_NON_SYM_NAT:

[pthatcher2, 2015/06/05 22:39:41]

NATTYPE_NON_SYMMETRIC?

[guoweis_webrtc, 2015/06/07 17:29:40]

Done.

+      return "Non-Symmetric NAT";

[pthatcher2, 2015/06/05 22:39:41]

Please capitalize all of them or none of them.

[guoweis_webrtc, 2015/06/07 17:29:40]

Done.

+    default:
+      return "unknown";

[pthatcher2, 2015/06/05 22:39:41]

Is this the same as NATTYPE_UNKNOWN?

[guoweis_webrtc, 2015/06/07 17:29:39]

It's invalid. Updated.

+  }
+}
+
 void PrintStats(StunProber* prober) {
   StunProber::Stats stats;
   if (!prober->GetStats(&stats)) {
     LOG(LS_WARNING) << "Results are inconclusive.";
     return;
   }
 
   LOG(LS_INFO) << "Shared Socket Mode: " << stats.shared_socket_mode;
   LOG(LS_INFO) << "Requests sent: " << stats.num_request_sent;
   LOG(LS_INFO) << "Responses received: " << stats.num_response_received;
   LOG(LS_INFO) << "Target interval (ns): " << stats.target_request_interval_ns;
   LOG(LS_INFO) << "Actual interval (ns): " << stats.actual_request_interval_ns;
-  LOG(LS_INFO) << "Behind NAT: " << stats.behind_nat;
-  if (stats.behind_nat) {
-    LOG(LS_INFO) << "NAT is symmetrical: " << (stats.srflx_addrs.size() > 1);
-  }
+  LOG(LS_INFO) << "NAT Type: " << PrintNATType(stats.nat_type);
   LOG(LS_INFO) << "Host IP: " << stats.host_ip;
   LOG(LS_INFO) << "Server-reflexive ips: ";
   for (auto& ip : stats.srflx_addrs) {
     LOG(LS_INFO) << "\t" << ip;
   }
 
-  std::string histogram_name = HistogramName(
-      stats.behind_nat, FLAG_shared_socket, FLAG_interval, "SuccessPercent");
+  std::string histogram_name =
+      HistogramName((stats.nat_type > NATTYPE::NATTYPE_NO_NAT),

[pthatcher2, 2015/06/05 22:39:41]

What if it's NATTYPE_UNKNOWN?

[guoweis_webrtc, 2015/06/07 17:29:40]

Remove the code. Histogram doesn't make sense here.

+                    FLAG_shared_socket, FLAG_interval, "SuccessPercent");
 
   LOG(LS_INFO) << "Histogram '" << histogram_name.c_str()
                << "' = " << stats.success_percent;
 
-  histogram_name = HistogramName(stats.behind_nat, FLAG_shared_socket,
-                                 FLAG_interval, "ResponseLatency");
+  histogram_name =
+      HistogramName((stats.nat_type > NATTYPE::NATTYPE_NO_NAT),

[pthatcher2, 2015/06/05 22:39:41]

You could avoid duplication with 

bool behind_nat = stats.nat_type > NATTYPE_UNKNOWN.

Or reuse the "is_behind_nat" function.

[guoweis_webrtc, 2015/06/07 17:29:39]

Remove the code. Histogram doesn't make sense here.

+                    FLAG_shared_socket, FLAG_interval, "ResponseLatency");
 
   LOG(LS_INFO) << "Histogram '" << histogram_name.c_str()
                << "' = " << stats.average_rtt_ms << " ms";
 }
 
 void StopTrial(rtc::Thread* thread, StunProber* prober, int result) {
   thread->Quit();
   if (prober) {
     LOG(LS_INFO) << "Result: " << result;
     if (result == StunProber::SUCCESS) {
@@ skipping 30 matching lines @@
                                       new SocketFactory(), new TaskRunner());
   auto finish_callback =
       [thread, prober](int result) { StopTrial(thread, prober, result); };
   prober->Start(server_addresses, FLAG_shared_socket, FLAG_interval,
                 FLAG_pings_per_ip, FLAG_timeout,
                 AsyncCallback(finish_callback));
   thread->Run();
   delete prober;
   return 0;
 }