bug 903: TapBridge doesn't close cleanly
authorTom Goff <thomas.goff@boeing.com>
Thu, 30 Dec 2010 14:19:48 -0800
changeset 6747 9dccf3624839
parent 6746 9724876b3ae1
child 6748 d1e3630ba7c4
bug 903: TapBridge doesn't close cleanly
src/core/unix-fd-reader.cc
src/core/unix-fd-reader.h
src/core/wscript
src/devices/tap-bridge/tap-bridge.cc
src/devices/tap-bridge/tap-bridge.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/src/core/unix-fd-reader.cc	Thu Dec 30 14:19:48 2010 -0800
@@ -0,0 +1,216 @@
+/* -*- Mode: C++; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */
+
+/*
+ * Copyright (c) 2010 The Boeing Company
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation;
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * Author: Tom Goff <thomas.goff@boeing.com>
+ */
+
+#include <errno.h>
+#include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+
+#include "ns3/log.h"
+#include "ns3/fatal-error.h"
+#include "ns3/simple-ref-count.h"
+#include "ns3/system-thread.h"
+#include "ns3/simulator.h"
+
+#include "unix-fd-reader.h"
+
+NS_LOG_COMPONENT_DEFINE ("FdReader");
+
+namespace ns3 {
+
+FdReader::FdReader ()
+  : m_fd (-1), m_readCallback (0), m_readThread (0), m_stop (false),
+    m_destroyEvent ()
+{
+  m_evpipe[0] = -1;
+  m_evpipe[1] = -1;
+}
+
+FdReader::~FdReader ()
+{
+  Stop ();
+}
+
+void FdReader::Start (int fd, Callback<void, uint8_t *, ssize_t> readCallback)
+{
+  int tmp;
+
+  NS_ASSERT_MSG (m_readThread == 0, "read thread already exists");
+
+  // create a pipe for inter-thread event notification
+  tmp = pipe (m_evpipe);
+  if (tmp == -1)
+    {
+      NS_FATAL_ERROR ("pipe() failed: " << strerror (errno));
+    }
+
+  // make the read end non-blocking
+  tmp = fcntl(m_evpipe[0], F_GETFL);
+  if (tmp == -1)
+    {
+      NS_FATAL_ERROR ("fcntl() failed: " << strerror (errno));
+    }
+  if (fcntl(m_evpipe[0], F_SETFL, tmp | O_NONBLOCK) == -1)
+    {
+      NS_FATAL_ERROR ("fcntl() failed: " << strerror (errno));
+    }
+
+  m_fd = fd;
+  m_readCallback = readCallback;
+
+  //
+  // We're going to spin up a thread soon, so we need to make sure we have
+  // a way to tear down that thread when the simulation stops.  Do this by
+  // scheduling a "destroy time" method to make sure the thread exits before
+  // proceeding.
+  //
+  if (! m_destroyEvent.IsRunning ())
+    {
+      // hold a reference to ensure that this object is not
+      // deallocated before the destroy-time event fires
+      this->Ref ();
+      m_destroyEvent =
+        Simulator::ScheduleDestroy (&FdReader::DestroyEvent, this);
+    }
+
+  //
+  // Now spin up a thread to read from the fd
+  //
+  NS_LOG_LOGIC ("Spinning up read thread");
+
+  m_readThread = Create<SystemThread> (MakeCallback (&FdReader::Run, this));
+  m_readThread->Start ();
+}
+
+void FdReader::DestroyEvent (void)
+{
+  Stop ();
+  this->Unref ();
+}
+
+void FdReader::Stop (void)
+{
+  m_stop = true;
+
+  // signal the read thread and close the write end of the event pipe
+  if (m_evpipe[1] != -1)
+    {
+      char zero = 0;
+      ssize_t len = write (m_evpipe[1], &zero, sizeof (zero));
+      if (len != sizeof (zero))
+        NS_LOG_WARN ("incomplete write(): " << strerror (errno));
+      close (m_evpipe[1]);
+      m_evpipe[1] = -1;
+    }
+
+  // join the read thread
+  if (m_readThread != 0)
+    {
+      m_readThread->Join ();
+      m_readThread = 0;
+    }
+
+  // close the read end of the event pipe
+  if (m_evpipe[0] != -1)
+    {
+      close (m_evpipe[0]);
+      m_evpipe[0] = -1;
+    }
+
+  // reset everything else
+  m_fd = -1;
+  m_readCallback.Nullify ();
+  m_stop = false;
+}
+
+// This runs in a separate thread
+void FdReader::Run (void)
+{
+  int nfds;
+  fd_set rfds;
+
+  nfds = (m_fd > m_evpipe[0] ? m_fd : m_evpipe[0]) + 1;
+
+  FD_ZERO (&rfds);
+  FD_SET (m_fd, &rfds);
+  FD_SET (m_evpipe[0], &rfds);
+
+  for (;;)
+    {
+      int r;
+      fd_set readfds = rfds;
+
+      r = select (nfds, &readfds, NULL, NULL, NULL);
+      if (r == -1 && errno != EINTR)
+        {
+          NS_FATAL_ERROR ("select() failed: " << strerror (errno));
+	}
+
+      if (FD_ISSET (m_evpipe[0], &readfds))
+        {
+          // drain the event pipe
+          ssize_t len;
+          for (;;)
+            {
+              char buf[1024];
+              len = read (m_evpipe[0], buf, sizeof (buf));
+              if (len == 0)
+                {
+                  NS_FATAL_ERROR ("event pipe closed");
+                }
+              if (len < 0)
+                {
+                  break;
+                }
+            }
+
+          if (len < 0 && errno != EAGAIN && errno != EWOULDBLOCK)
+            {
+              NS_LOG_WARN ("read() failed: " << strerror (errno));
+              break;
+            }
+	}
+
+      if (m_stop)
+        {
+          // this thread is done
+          break;
+        }
+
+      if (FD_ISSET (m_fd, &readfds))
+        {
+          struct FdReader::Data data = DoRead ();
+          // reading stops when m_len is zero
+          if (data.m_len == 0)
+            {
+              break;
+            }
+          // the callback is only called when m_len is positive (data
+          // is ignored if m_len is negative)
+          else if (data.m_len > 0)
+            {
+              m_readCallback (data.m_buf, data.m_len);
+            }
+        }
+    }
+}
+
+} // namespace ns3
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/src/core/unix-fd-reader.h	Thu Dec 30 14:19:48 2010 -0800
@@ -0,0 +1,113 @@
+/* -*- Mode: C++; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */
+
+/*
+ * Copyright (c) 2010 The Boeing Company
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation;
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * Author: Tom Goff <thomas.goff@boeing.com>
+ */
+
+#ifndef UNIX_FD_READER_H
+#define UNIX_FD_READER_H
+
+#include <stdint.h>
+
+#include "ns3/callback.h"
+#include "ns3/system-thread.h"
+#include "ns3/event-id.h"
+
+namespace ns3 {
+
+/**
+ * \brief A class that asynchronously reads from a file descriptor.
+ *
+ * This class can be used to start a system thread that reads from a
+ * given file descriptor and invokes a given callback when data is
+ * received.  This class handles thread management automatically but
+ * the \p DoRead() method must be implemented by a subclass.
+ */
+class FdReader : public SimpleRefCount<FdReader>
+{
+public:
+  FdReader();
+  ~FdReader();
+
+  /**
+   * Start a new read thread.
+   *
+   * \param fd A valid file descriptor open for reading.
+   *
+   * \param readCallback A callback to invoke when new data is
+   * available.
+   */
+  void Start (int fd, Callback<void, uint8_t *, ssize_t> readCallback);
+
+  /**
+   * Stop the read thread and reset internal state.  This does not
+   * close the file descriptor used for reading.
+   */
+  void Stop (void);
+
+protected:
+
+  /**
+   * \internal
+   * \brief A structure representing data read.
+   */
+  struct Data
+  {
+    Data () : m_buf (0), m_len (0) {}
+    Data (uint8_t *buf, ssize_t len) : m_buf (buf), m_len (len) {}
+    uint8_t *m_buf;
+    ssize_t m_len;
+  };
+
+  /**
+   * \internal
+   * \brief The read implementation.
+   *
+   * The value of \p m_len returned controls further processing.  The
+   * callback function is only invoked when \p m_len is positive; any
+   * data read is not processed when \p m_len is negative; reading
+   * stops when \p m_len is zero.
+   *
+   * The management of memory associated with \p m_buf must be
+   * compatible with the read callback.
+   *
+   * \return A structure representing what was read.
+   */
+  virtual FdReader::Data DoRead (void) = 0;
+
+  /**
+   * \internal
+   * \brief The file descriptor to read from.
+   */
+  int m_fd;
+
+private:
+
+  void Run (void);
+  void DestroyEvent (void);
+
+  Callback<void, uint8_t *, ssize_t> m_readCallback;
+  Ptr<SystemThread> m_readThread;
+  int m_evpipe[2];           // pipe used to signal events between threads
+  bool m_stop;               // true means the read thread should stop
+  EventId m_destroyEvent;
+};
+
+} // namespace ns3
+
+#endif	// UNIX_FD_READER_H
--- a/src/core/wscript	Thu Dec 30 16:04:47 2010 -0800
+++ b/src/core/wscript	Thu Dec 30 14:19:48 2010 -0800
@@ -146,12 +146,14 @@
             'unix-system-thread.cc',
             'unix-system-mutex.cc',
             'unix-system-condition.cc',
+            'unix-fd-reader.cc',
             ])
         core.uselib = 'PTHREAD'
         headers.source.extend([
                 'system-mutex.h',
                 'system-thread.h',
                 'system-condition.h',
+                'unix-fd-reader.h',
                 ])
 
     if bld.env['ENABLE_GSL']:
--- a/src/devices/tap-bridge/tap-bridge.cc	Thu Dec 30 16:04:47 2010 -0800
+++ b/src/devices/tap-bridge/tap-bridge.cc	Thu Dec 30 14:19:48 2010 -0800
@@ -32,7 +32,7 @@
 #include "ns3/ipv4.h"
 #include "ns3/simulator.h"
 #include "ns3/realtime-simulator-impl.h"
-#include "ns3/system-thread.h"
+#include "ns3/unix-fd-reader.h"
 #include "ns3/uinteger.h"
 
 #include <sys/wait.h>
@@ -65,6 +65,27 @@
 
 namespace ns3 {
 
+FdReader::Data TapBridgeFdReader::DoRead (void)
+{
+  NS_LOG_FUNCTION_NOARGS ();
+
+  uint32_t bufferSize = 65536;
+  uint8_t *buf = (uint8_t *)malloc (bufferSize);
+  NS_ABORT_MSG_IF (buf == 0, "malloc() failed");
+
+  NS_LOG_LOGIC ("Calling read on tap device fd " << m_fd);
+  ssize_t len = read (m_fd, buf, bufferSize);
+  if (len <= 0)
+    {
+      NS_LOG_INFO ("TapBridgeFdReader::DoRead(): done");
+      free (buf);
+      buf = 0;
+      len = 0;
+    }
+
+  return FdReader::Data (buf, len);
+}
+
 #define TAP_MAGIC 95549
 
 NS_OBJECT_ENSURE_REGISTERED (TapBridge);
@@ -135,7 +156,7 @@
   m_sock (-1),
   m_startEvent (),
   m_stopEvent (),
-  m_readThread (0),
+  m_fdReader (0),
   m_ns3AddressRewritten (false)
 {
   NS_LOG_FUNCTION_NOARGS ();
@@ -147,6 +168,8 @@
 {
   NS_LOG_FUNCTION_NOARGS ();
 
+  StopTapDevice ();
+
   delete [] m_packetBuffer;
   m_packetBuffer = 0;
 
@@ -235,11 +258,11 @@
   //
   // Now spin up a read thread to read packets from the tap device.
   //
-  NS_ABORT_MSG_IF (m_readThread != 0,"TapBridge::StartTapDevice(): Receive thread is already running");
+  NS_ABORT_MSG_IF (m_fdReader != 0,"TapBridge::StartTapDevice(): Receive thread is already running");
   NS_LOG_LOGIC ("Spinning up read thread");
 
-  m_readThread = Create<SystemThread> (MakeCallback (&TapBridge::ReadThread, this));
-  m_readThread->Start ();
+  m_fdReader = Create<TapBridgeFdReader> ();
+  m_fdReader->Start (m_sock, MakeCallback (&TapBridge::ReadCallback, this));
 }
 
 void
@@ -247,14 +270,17 @@
 {
   NS_LOG_FUNCTION_NOARGS ();
 
-  close (m_sock);
-  m_sock = -1;
+  if (m_fdReader != 0)
+    {
+      m_fdReader->Stop ();
+      m_fdReader = 0;
+    }
 
-  NS_ASSERT_MSG (m_readThread != 0, "TapBridge::StopTapDevice(): Receive thread is not running");
-
-  NS_LOG_LOGIC ("Joining read thread");
-  m_readThread->Join ();
-  m_readThread = 0;
+  if (m_sock != -1)
+    {
+      close (m_sock);
+      m_sock = -1;
+    }
 }
 
 void
@@ -636,49 +662,33 @@
 }
 
 void
-TapBridge::ReadThread (void)
+TapBridge::ReadCallback (uint8_t *buf, ssize_t len)
 {
   NS_LOG_FUNCTION_NOARGS ();
 
+  NS_ASSERT_MSG (buf != 0, "invalid buf argument");
+  NS_ASSERT_MSG (len > 0, "invalid len argument");
+
   //
-  // It's important to remember that we're in a completely different thread 
-  // than the simulator is running in.  We need to synchronize with that 
-  // other thread to get the packet up into ns-3.  What we will need to do 
+  // It's important to remember that we're in a completely different thread
+  // than the simulator is running in.  We need to synchronize with that
+  // other thread to get the packet up into ns-3.  What we will need to do
   // is to schedule a method to deal with the packet using the multithreaded
   // simulator we are most certainly running.  However, I just said it -- we
   // are talking about two threads here, so it is very, very dangerous to do
   // any kind of reference counting on a shared object.  Just don't do it.
-  // So what we're going to do is to allocate a buffer on the heap and pass
-  // that buffer into the ns-3 context thread where it will create the packet.
+  // So what we're going to do is pass the buffer allocated on the heap
+  // into the ns-3 context thread where it will create the packet.
   //
-  int32_t len = -1;
-
-  for (;;) 
-    {
-      uint32_t bufferSize = 65536;
-      uint8_t *buf = (uint8_t *)malloc (bufferSize);
-      NS_ABORT_MSG_IF (buf == 0, "TapBridge::ReadThread(): malloc packet buffer failed");
-      NS_LOG_LOGIC ("Calling read on tap device socket fd " << m_sock);
-      len = read (m_sock, buf, bufferSize);
 
-      if (len == -1)
-        {
-          NS_LOG_INFO ("TapBridge::ReadThread(): Returning");
-          free (buf);
-          buf = 0;
-          return;
-        }
-
-      NS_LOG_INFO ("TapBridge::ReadThread(): Received packet on node " << m_nodeId);
-      NS_LOG_INFO ("TapBridge::ReadThread(): Scheduling handler");
-      NS_ASSERT_MSG (m_rtImpl, "EmuNetDevice::ReadThread(): Realtime simulator implementation pointer not set");
-      m_rtImpl->ScheduleRealtimeNowWithContext (m_nodeId, MakeEvent (&TapBridge::ForwardToBridgedDevice, this, buf, len));
-      buf = 0;
-    }
+  NS_LOG_INFO ("TapBridge::ReadCallback(): Received packet on node " << m_nodeId);
+  NS_LOG_INFO ("TapBridge::ReadCallback(): Scheduling handler");
+  NS_ASSERT_MSG (m_rtImpl, "TapBridge::ReadCallback(): Realtime simulator implementation pointer not set");
+  m_rtImpl->ScheduleRealtimeNowWithContext (m_nodeId, MakeEvent (&TapBridge::ForwardToBridgedDevice, this, buf, len));
 }
 
 void
-TapBridge::ForwardToBridgedDevice (uint8_t *buf, uint32_t len)
+TapBridge::ForwardToBridgedDevice (uint8_t *buf, ssize_t len)
 {
   NS_LOG_FUNCTION (buf << len);
 
--- a/src/devices/tap-bridge/tap-bridge.h	Thu Dec 30 16:04:47 2010 -0800
+++ b/src/devices/tap-bridge/tap-bridge.h	Thu Dec 30 14:19:48 2010 -0800
@@ -31,11 +31,17 @@
 #include "ns3/data-rate.h"
 #include "ns3/ptr.h"
 #include "ns3/mac48-address.h"
-#include "ns3/system-thread.h"
+#include "ns3/unix-fd-reader.h"
 #include "ns3/realtime-simulator-impl.h"
 
 namespace ns3 {
 
+class TapBridgeFdReader : public FdReader
+{
+private:
+  FdReader::Data DoRead (void);
+};
+
 class Node;
 
 /**
@@ -248,9 +254,9 @@
   /**
    * \internal
    *
-   * Loop to read and process packets
+   * Callback to process packets that are read
    */
-  void ReadThread (void);
+  void ReadCallback (uint8_t *buf, ssize_t len);
 
   /*
    * \internal
@@ -262,7 +268,7 @@
    *            received from the host.
    * \param buf The length of the buffer.
    */
-  void ForwardToBridgedDevice (uint8_t *buf, uint32_t len);
+  void ForwardToBridgedDevice (uint8_t *buf, ssize_t len);
 
   /**
    * \internal
@@ -336,7 +342,7 @@
    * The socket (actually interpreted as fd) to use to talk to the Tap device on
    * the real internet host.
    */
-  int32_t m_sock;
+  int m_sock;
 
   /**
    * \internal
@@ -357,10 +363,10 @@
   /**
    * \internal
    *
-   * Used to identify the ns-3 read thread used to do blocking reads on the 
-   * socket (fd) corresponding to the host device.
+   * Includes the ns-3 read thread used to do blocking reads on the fd
+   * corresponding to the host device.
    */
-  Ptr<SystemThread> m_readThread;
+  Ptr<TapBridgeFdReader> m_fdReader;
 
   /**
    * \internal