Merge pull request #1433 from fwsGonzo/dev

Various TCP fixes

Author: fwsGonzo

api/net/ip4/addr.hpp

@@ -107,10 +107,11 @@ struct Addr {
 
     const static std::regex ipv4_address_pattern
     {
-      "^\\s*(25[0-5]|2[0-4]\\d|[01]?\\d\\d?)\\."
-           "(25[0-5]|2[0-4]\\d|[01]?\\d\\d?)\\."
-           "(25[0-5]|2[0-4]\\d|[01]?\\d\\d?)\\."
-           "(25[0-5]|2[0-4]\\d|[01]?\\d\\d?)\\s*$"
+
+#define OCTET_PATTERN "(25[0-5]|2[0-4]\\d|[01]?\\d\\d?)"
+      "^\\s*" OCTET_PATTERN "\\." OCTET_PATTERN "\\." OCTET_PATTERN "\\." OCTET_PATTERN "\\s*$"
+#undef OCTET_PATTERN
+
     };
 
     std::smatch ipv4_parts;

api/net/tcp/tcp.hpp

@@ -26,7 +26,7 @@
 #include "packet.hpp"
 
 #include <map>  // connections, listeners
-#include <queue>  // writeq
+#include <deque>  // writeq
 #include <net/inet.hpp>
 #include <net/socket.hpp>
 #include <net/port_util.hpp>
@@ -710,7 +710,7 @@ namespace net {
      *
      * @param[in]  <unnamed>  A ptr to a Connection
      */
-    void queue_offer(tcp::Connection_ptr);
+    void queue_offer(tcp::Connection&);
 
     /**
      * @brief      Force the TCP to process the it's queue with the current amount of available packets.

api/util/statman.hpp

@@ -25,38 +25,27 @@
 #include <membitmap>
 #include <smp_utils>
 
-/** This type is thrown when Statman's span is full */
 struct Stats_out_of_memory : public std::out_of_range {
   explicit Stats_out_of_memory()
-    : std::out_of_range(std::string{"Statman has no room for more statistics"})
+    : std::out_of_range("Statman has no room for more statistics")
     {}
-}; //< struct Stats_out_of_memory
+};
 
-
-/** This type is thrown from within the operations of class Statman */
 struct Stats_exception : public std::runtime_error {
   using runtime_error::runtime_error;
-}; //< struct Stats_exception
+};
 
-///
-///
-///
 class Stat {
 public:
-  static const int MAX_NAME_LEN {47};
-  ///
-  ///
-  ///
+  static const int MAX_NAME_LEN = 47;
+
   enum Stat_type: uint8_t
   {
     FLOAT,
     UINT32,
     UINT64
   };
 
-  ///
-  ///
-  ///
   Stat(const Stat_type type, const std::string& name);
   ~Stat() = default;
 
@@ -93,7 +82,7 @@ class Stat {
   };
   Stat_type type_;
 
-  char name_[MAX_NAME_LEN];
+  char name_[MAX_NAME_LEN+1];
 
   Stat(const Stat& other) = delete;
   Stat(const Stat&& other) = delete;

src/kernel/timers.cpp

@@ -65,10 +65,10 @@ struct alignas(SMP_ALIGN) timer_system
   // timers sorted by timestamp
   std::multimap<duration_t, Timers::id_t> scheduled;
   /** Stats */
-  int64_t*  oneshot_started;
-  int64_t*  oneshot_stopped;
-  uint32_t* periodic_started;
-  uint32_t* periodic_stopped;
+  int64_t*  oneshot_started = nullptr;
+  int64_t*  oneshot_stopped = nullptr;
+  uint32_t* periodic_started = nullptr;
+  uint32_t* periodic_stopped = nullptr;
 };
 static SMP_ARRAY<timer_system> systems;
 
@@ -83,7 +83,7 @@ void Timers::init(const start_func_t& start, const stop_func_t& stop)
   system.arch_start_func = start;
   system.arch_stop_func  = stop;
 
-  std::string CPU = "cpu" + std::to_string(SMP::cpu_id());
+  const std::string CPU = "cpu" + std::to_string(SMP::cpu_id());
   system.oneshot_started = (int64_t*) &Statman::get().create(Stat::UINT64, CPU + ".timers.oneshot_started").get_uint64();
   system.oneshot_stopped = (int64_t*) &Statman::get().create(Stat::UINT64, CPU + ".timers.oneshot_stopped").get_uint64();
   system.periodic_started = &Statman::get().create(Stat::UINT32, CPU + ".timers.periodic_started").get_uint32();

src/net/tcp/connection.cpp

@@ -143,6 +143,10 @@ size_t Connection::receive(seq_t seq, const uint8_t* data, size_t n, bool PUSH)
 
 void Connection::write(Chunk buffer)
 {
+  if (UNLIKELY(buffer.size() == 0)) {
+    throw TCP_error("Can't write zero bytes to TCP stream");
+  }
+
   // Only write if allowed
   if(state_->is_writable())
   {
@@ -199,9 +203,9 @@ void Connection::offer(size_t& packets)
   debug2("<Connection::offer> Finished working offer with [%u] packets left and a queue of (%u) with a usable window of %i\n",
         packets, writeq.size(), usable_window());
 
-  if(can_send() and not queued_)
+  if (this->can_send() and not this->is_queued())
   {
-    host_.queue_offer(retrieve_shared());
+    host_.queue_offer(*this);
   }
 }
 
@@ -267,8 +271,11 @@ void Connection::receive_disconnect() {
   Expects(read_request and read_request->buffer.buffer());
   auto& buf = read_request->buffer;
 
-  if(read_request->callback)
-    read_request->callback(buf.buffer(), buf.size());
+  if(read_request->callback) {
+    // TODO: consider adding back when SACK is complete
+    //if (buf.size() > 0 && buf.missing() == 0)
+    //    read_request->callback(buf.buffer(), buf.size());
+  }
 }
 
 void Connection::segment_arrived(Packet_ptr incoming)

src/net/tcp/tcp.cpp

@@ -489,15 +489,25 @@ void TCP::request_offer(Connection& conn) {
   debug2("<TCP::request_offer> %s requestin offer: uw=%u rem=%u\n",
     conn.to_string().c_str(), conn.usable_window(), conn.sendq_remaining());
 
+  // Note: Must be called even if packets is 0
+  // because the connectoin is responsible for requeuing itself (see Connection::offer)
   conn.offer(packets);
 }
 
-void TCP::queue_offer(Connection_ptr conn)
+
+void TCP::queue_offer(Connection& conn)
 {
-  if(not conn->is_queued() and conn->can_send())
+  if(not conn.is_queued() and conn.can_send())
   {
-    debug("<TCP::queue_offer> %s queued\n", conn->to_string().c_str());
-    writeq.push_back(conn);
-    conn->set_queued(true);
+    try {
+      debug("<TCP::queue_offer> %s queued\n", conn.to_string().c_str());
+      writeq.push_back(conn.retrieve_shared());
+      conn.set_queued(true);
+    }
+    catch (std::exception& e) {
+      printf("ERROR: Could not find connection for %p: %s\n",
+            &conn, conn.to_string().c_str());
+      throw;
+    }
   }
 }

src/util/statman.cpp

@@ -28,18 +28,11 @@ Statman& Statman::get() {
 
 ///////////////////////////////////////////////////////////////////////////////
 Stat::Stat(const Stat_type type, const std::string& name)
-  : type_{type}
+  : ui64(0), type_{type}
 {
   if(name.size() > MAX_NAME_LEN)
     throw Stats_exception{"Creating stat: Name cannot be longer than " + std::to_string(MAX_NAME_LEN) + " characters"};
 
-  switch (type) {
-    case UINT32: ui32 = 0;    break;
-    case UINT64: ui64 = 0;    break;
-    case FLOAT:  f    = 0.0f; break;
-    default: throw Stats_exception{"Unimplemented stat type"};
-  }
-
   snprintf(name_, sizeof(name_), "%s", name.c_str());
 }
 
@@ -79,7 +72,7 @@ uint64_t& Stat::get_uint64() {
 void Statman::init(const uintptr_t start, const Size_type num_bytes)
 {
   if (num_bytes < 0)
-    throw Stats_exception{"Creating Statman: A negative number of bytes has been given"};
+    throw Stats_exception("Can't initialize statman with negative size");
 
   const int N = num_bytes / sizeof(Stat);
   this->stats_     = reinterpret_cast<Stat*>(start);
@@ -103,7 +96,7 @@ Stat& Statman::create(const Stat::Stat_type type, const std::string& name) {
   volatile scoped_spinlock lock(this->stlock);
 #endif
   if (name.empty())
-    throw Stats_exception{"Cannot create Stat with no name"};
+    throw Stats_exception("Cannot create Stat with no name");
 
   const int idx = bitmap.first_free();
   if (idx == -1 || idx >= capacity())

test/net/unit/ip4_addr.cpp

@@ -96,6 +96,14 @@ CASE("Create IP4 addresses from strings")
   }
   **/
 
+  EXPECT_NO_THROW(Addr("202.209.27.78"));
+  EXPECT_NO_THROW(Addr("212.209.27.78"));
+  EXPECT_NO_THROW(Addr("222.209.27.78"));
+  EXPECT_NO_THROW(Addr("232.209.27.78"));
+  EXPECT_NO_THROW(Addr("242.209.27.78"));
+  EXPECT_NO_THROW(Addr("255.209.27.78"));
+  EXPECT_THROWS(Addr("265.209.27.78"));
+  EXPECT_THROWS(Addr("256.209.27.78"));
   EXPECT_THROWS(Addr{"LUL"});
   EXPECT_THROWS(Addr{"12310298310298301283"});
   EXPECT_THROWS(const Addr invalid1{"256.256.256.256"});