Feature (contd.): Device Emulation - Got rid of a bunch of FIXMEs and all trailing whitespace (in the code added for this feature)

server/abstractport.h

@@ -30,6 +30,9 @@ class StreamBase;
 class PacketBuffer;
 class QIODevice;
 
+// TODO: send notification back to client(s)
+#define notify qWarning
+
 class AbstractPort
 {
 public:

server/device.cpp

@@ -30,9 +30,9 @@ const int kBaseHex = 16;
 const quint64 kBcastMac = 0xffffffffffffULL;
 
 /*
- * NOTE: 
+ * NOTE:
  * 1. Device Key is (VLANS + MAC) - is assumed to be unique for a device
- * 2. Device clients/users (viz. DeviceManager) should take care when 
+ * 2. Device clients/users (viz. DeviceManager) should take care when
  *    setting params that change the key, if the key is used elsewhere
  *    (e.g. in a hash)
  */
@@ -117,14 +117,14 @@ DeviceKey Device::key()
 }
 
 void Device::clearKey()
-{ 
+{
     key_.fill(0, kMaxVlan * sizeof(quint16) + sizeof(quint64));
 }
 
 int Device::encapSize()
 {
     // ethernet header + vlans
-    int size = 14 + 4*numVlanTags_; 
+    int size = 14 + 4*numVlanTags_;
 
     return size;
 }
@@ -132,7 +132,7 @@ int Device::encapSize()
 void Device::encap(PacketBuffer *pktBuf, quint64 dstMac, quint16 type)
 {
     int ofs;
-    quint64 srcMac = mac_; 
+    quint64 srcMac = mac_;
     uchar *p = pktBuf->push(encapSize());
 
     if (!p) {
@@ -306,7 +306,7 @@ void Device::receiveArp(PacketBuffer *pktBuf)
     // Extract tgtIp first to check quickly if this packet is for us or not
     tgtIp = qFromBigEndian<quint32>(pktData + 24);
     if (tgtIp != ip4_) {
-        qDebug("tgtIp %s is not me %s", 
+        qDebug("tgtIp %s is not me %s",
                 qPrintable(QHostAddress(tgtIp).toString()),
                 qPrintable(QHostAddress(ip4_).toString()));
         return;
@@ -355,7 +355,7 @@ void Device::receiveArp(PacketBuffer *pktBuf)
         arpTable.insert(srcIp, srcMac);
 
         rspPkt = new PacketBuffer;
-        rspPkt->reserve(encapSize()); 
+        rspPkt->reserve(encapSize());
         pktData = rspPkt->put(28);
         if (pktData) {
             // HTYP, PTYP
@@ -376,7 +376,7 @@ void Device::receiveArp(PacketBuffer *pktBuf)
         transmitPacket(rspPkt);
 
         qDebug("Sent ARP Reply for srcIp/tgtIp=%s/%s",
-                qPrintable(QHostAddress(srcIp).toString()), 
+                qPrintable(QHostAddress(srcIp).toString()),
                 qPrintable(QHostAddress(tgtIp).toString()));
         break;
     case 2: // ARP Response

server/devicemanager.cpp

@@ -32,7 +32,8 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>
 
 const quint64 kBcastMac = 0xffffffffffffULL;
 
-// FIXME: add lock to protect deviceGroupList_ operations?
+// XXX: Port owning DeviceManager already uses locks, so we don't use any
+// locks within DeviceManager to protect deviceGroupList_ et.al.
 
 DeviceManager::DeviceManager(AbstractPort *parent)
 {
@@ -75,7 +76,7 @@ bool DeviceManager::addDeviceGroup(uint deviceGroupId)
     OstProto::DeviceGroup *deviceGroup;
 
     if (deviceGroupList_.contains(deviceGroupId)) {
-        qWarning("%s: deviceGroup id %u already exists", __FUNCTION__, 
+        qWarning("%s: deviceGroup id %u already exists", __FUNCTION__,
                 deviceGroupId);
         return false;
     }
@@ -97,7 +98,7 @@ bool DeviceManager::deleteDeviceGroup(uint deviceGroupId)
 {
     OstProto::DeviceGroup *deviceGroup;
     if (!deviceGroupList_.contains(deviceGroupId)) {
-        qWarning("%s: deviceGroup id %u does not exist", __FUNCTION__, 
+        qWarning("%s: deviceGroup id %u does not exist", __FUNCTION__,
                 deviceGroupId);
         return false;
     }
@@ -137,8 +138,6 @@ int DeviceManager::deviceCount()
 void DeviceManager::getDeviceList(
         OstProto::PortDeviceList *deviceList)
 {
-    int count = 0;
-
     foreach(Device *device, deviceList_) {
         OstEmul::Device *dev =
             deviceList->AddExtension(OstEmul::port_device);
@@ -160,7 +159,11 @@ void DeviceManager::receivePacket(PacketBuffer *pktBuf)
     // We assume pkt is ethernet
     // TODO: extend for other link layer types
 
-    // FIXME: validate before extracting if the offset is within pktLen
+    // All frames we are interested in should be at least 32 bytes
+    if (pktBuf->length() < 32) {
+        qWarning("short frame of %d bytes, skipping ...", pktBuf->length());
+        return;
+    }
 
     // Extract dstMac
     dstMac = qFromBigEndian<quint32>(pktData + offset);

server/devicemanager.h

@@ -32,7 +32,7 @@ namespace OstProto {
     class DeviceGroup;
 };
 
-class DeviceManager 
+class DeviceManager
 {
 public:
     DeviceManager(AbstractPort *parent = 0);
@@ -66,7 +66,7 @@ private:
     void enumerateDevices(
             const OstProto::DeviceGroup *deviceGroup,
             Operation oper);
-             
+
     AbstractPort *port_;
     QHash<uint, OstProto::DeviceGroup*> deviceGroupList_;
     QHash<DeviceKey, Device*> deviceList_;

server/drone.pro

@@ -45,7 +45,7 @@ SOURCES += \
     winpcapport.cpp 
 SOURCES += myservice.cpp 
 SOURCES += pcapextra.cpp 
-SOURCES += packetbuffer.cpp 
+SOURCES += packetbuffer.cpp
 
 QMAKE_DISTCLEAN += object_script.*
 

server/myservice.cpp

@@ -599,12 +599,15 @@ _invalid_version:
     done->Run();
 }
 
-/* 
+/*
  * ===================================================================
  * Device Emulation
- * FIXME: Locking for these functions is at Port level, should it be
+ * ===================================================================
- *        moved to inside DeviceManager instead? In other words, are
+ * XXX: Streams and Devices are largely non-overlapping from a RPC
- *        streams/ports and devices independent?
+ * point of view but they *do* intersect e.g. when a stream is trying to
+ * find its associated device and info from that device such as src/dst
+ * mac addresses. For this reason, both set of RPCs share the common
+ * port level locking
  * ===================================================================
  */
 void MyService::getDeviceGroupIdList(
@@ -699,10 +702,8 @@ void MyService::addDeviceGroup(
 
     devMgr = portInfo[portId]->deviceManager();
 
-#if 0 // FIXME: needed?
     if (portInfo[portId]->isTransmitOn())
         goto _port_busy;
-#endif
 
     portLock[portId]->lockForWrite();
     for (int i = 0; i < request->device_group_id_size(); i++)
@@ -723,11 +724,9 @@ void MyService::addDeviceGroup(
     done->Run();
     return;
 
-#if 0 // FIXME: needed?
 _port_busy:
     controller->SetFailed("Port Busy");
     goto _exit;
-#endif
 
 _invalid_port:
     controller->SetFailed("invalid portid");
@@ -752,10 +751,8 @@ void MyService::deleteDeviceGroup(
 
     devMgr = portInfo[portId]->deviceManager();
 
-#if 0 // FIXME: needed?
     if (portInfo[portId]->isTransmitOn())
         goto _port_busy;
-#endif
 
     portLock[portId]->lockForWrite();
     for (int i = 0; i < request->device_group_id_size(); i++)
@@ -767,11 +764,9 @@ void MyService::deleteDeviceGroup(
     done->Run();
     return;
 
-#if 0 // FIXME: needed?
 _port_busy:
     controller->SetFailed("Port Busy");
     goto _exit;
-#endif
 _invalid_port:
     controller->SetFailed("invalid portid");
 _exit:
@@ -795,28 +790,22 @@ void MyService::modifyDeviceGroup(
 
     devMgr = portInfo[portId]->deviceManager();
 
-#if 0 // FIXME: needed?
     if (portInfo[portId]->isTransmitOn())
         goto _port_busy;
-#endif
 
     portLock[portId]->lockForWrite();
     for (int i = 0; i < request->device_group_size(); i++)
         devMgr->modifyDeviceGroup(&request->device_group(i));
     portLock[portId]->unlock();
 
-    // FIXME: check for overlaps between devices?
-
     //! \todo(LOW): fill-in response "Ack"????
 
     done->Run();
     return;
 
-#if 0 // FIXME: needed?
 _port_busy:
     controller->SetFailed("Port Busy");
     goto _exit;
-#endif
 _invalid_port:
     controller->SetFailed("invalid portid");
 _exit:

server/pcapport.cpp

@@ -84,7 +84,7 @@ PcapPort::PcapPort(int id, const char *device)
     monitorTx_ = new PortMonitor(device, kDirectionTx, &stats_);
     transmitter_ = new PortTransmitter(device);
     capturer_ = new PortCapturer(device);
-    receiver_ = new PortReceiver(device, deviceManager_);
+    emulXcvr_ = new EmulationTransceiver(device, deviceManager_);
 
     if (!monitorRx_->handle() || !monitorTx_->handle())
         isUsable_ = false;
@@ -137,7 +137,7 @@ PcapPort::~PcapPort()
     if (monitorTx_)
         monitorTx_->stop();
 
-    delete receiver_;
+    delete emulXcvr_;
     delete capturer_;
     delete transmitter_;
 
@@ -174,17 +174,17 @@ void PcapPort::updateNotes()
 
 void PcapPort::startDeviceEmulation()
 {
-    receiver_->start();
+    emulXcvr_->start();
 }
 
 void PcapPort::stopDeviceEmulation()
 {
-    receiver_->stop();
+    emulXcvr_->stop();
 }
 
 int PcapPort::sendEmulationPacket(PacketBuffer *pktBuf)
 {
-    return receiver_->transmitPacket(pktBuf);
+    return emulXcvr_->transmitPacket(pktBuf);
 }
 
 
@@ -895,10 +895,10 @@ QFile* PcapPort::PortCapturer::captureFile()
 
 /*
  * ------------------------------------------------------------------- *
- * Port Receiver
+ * Transmit+Receiver for Device/ProtocolEmulation
  * ------------------------------------------------------------------- *
  */
-PcapPort::PortReceiver::PortReceiver(const char *device, 
+PcapPort::EmulationTransceiver::EmulationTransceiver(const char *device,
         DeviceManager *deviceManager)
 {
     device_ = QString::fromAscii(device);
@@ -908,19 +908,19 @@ PcapPort::PortReceiver::PortReceiver(const char *device,
     handle_ = NULL;
 }
 
-PcapPort::PortReceiver::~PortReceiver()
+PcapPort::EmulationTransceiver::~EmulationTransceiver()
 {
     stop();
 }
 
-void PcapPort::PortReceiver::run()
+void PcapPort::EmulationTransceiver::run()
 {
     int flags = PCAP_OPENFLAG_PROMISCUOUS;
     char errbuf[PCAP_ERRBUF_SIZE] = "";
     struct bpf_program bpf;
     const char *capture_filter = "arp or (vlan and arp)";
     const int optimize = 1;
-    
+
     qDebug("In %s", __PRETTY_FUNCTION__);
 
 #ifdef Q_OS_WIN32
@@ -931,10 +931,10 @@ _retry:
     // FIXME: use 0 timeout value?
 #ifdef Q_OS_WIN32
     // NOCAPTURE_LOCAL needs windows only pcap_open()
-    handle_ = pcap_open(qPrintable(device_), 65535, 
+    handle_ = pcap_open(qPrintable(device_), 65535,
                 flags, 1000 /* ms */, NULL, errbuf);
 #else
-    handle_ = pcap_open_live(qPrintable(device_), 65535, 
+    handle_ = pcap_open_live(qPrintable(device_), 65535,
                     flags, 1000 /* ms */, errbuf);
 #endif
 
@@ -942,14 +942,12 @@ _retry:
     {
         if (flags && QString(errbuf).contains("promiscuous"))
         {
-            // FIXME: warn user that device emulation will not work
+            notify("Unable to set promiscuous mode on <%s> - "
-            qDebug("%s:can't set promiscuous mode, trying non-promisc", 
+                    "device emulation will not work", qPrintable(device_));
-                    qPrintable(device_));
+            goto _exit;
-            flags &= ~PCAP_OPENFLAG_PROMISCUOUS;
-            goto _retry;
         }
 #ifdef Q_OS_WIN32
-        else if ((flags & PCAP_OPENFLAG_NOCAPTURE_LOCAL) 
+        else if ((flags & PCAP_OPENFLAG_NOCAPTURE_LOCAL)
                 && QString(errbuf).contains("loopback"))
         {
             qDebug("Can't set no local capture mode %s", qPrintable(device_));
@@ -959,22 +957,21 @@ _retry:
 #endif
         else
         {
-            // FIXME: warn user that device emulation will not work
+            notify("Unable to open <%s> [%s] - device emulation will not work",
-            qDebug("%s: Error opening port %s: %s\n", __FUNCTION__,
+                    qPrintable(device_), errbuf);
-                    device_.toAscii().constData(), errbuf);
             goto _exit;
         }
     }
 
     // FIXME: hardcoded filter
-    if (pcap_compile(handle_, &bpf, capture_filter, optimize, 0) < 0) 
+    if (pcap_compile(handle_, &bpf, capture_filter, optimize, 0) < 0)
     {
         qWarning("%s: error compiling filter: %s", qPrintable(device_),
                 pcap_geterr(handle_));
         goto _skip_filter;
     }
 
-    if (pcap_setfilter(handle_, &bpf) < 0) 
+    if (pcap_setfilter(handle_, &bpf) < 0)
     {
         qWarning("%s: error setting filter: %s", qPrintable(device_),
                 pcap_geterr(handle_));
@@ -992,32 +989,32 @@ _skip_filter:
         ret = pcap_next_ex(handle_, &hdr, &data);
         switch (ret)
         {
-            case 1: 
+            case 1:
             {
                 PacketBuffer *pktBuf = new PacketBuffer(data, hdr->caplen);
 
                 // XXX: deviceManager should free pktBuf before returning
                 // from this call; if it needs to process the pkt async
                 // it should make a copy as the pktBuf's data buffer is
-                // owned by libpcap which does not guarantee data will 
+                // owned by libpcap which does not guarantee data will
                 // persist across calls to pcap_next_ex()
-                deviceManager_->receivePacket(pktBuf); 
+                deviceManager_->receivePacket(pktBuf);
                 break;
             }
             case 0:
                 // timeout: just go back to the loop
                 break;
             case -1:
-                qWarning("%s: error reading packet (%d): %s", 
+                qWarning("%s: error reading packet (%d): %s",
                         __PRETTY_FUNCTION__, ret, pcap_geterr(handle_));
                 break;
             case -2:
             default:
-                qFatal("%s: Unexpected return value %d", __PRETTY_FUNCTION__, 
+                qFatal("%s: Unexpected return value %d", __PRETTY_FUNCTION__,
                         ret);
         }
 
-        if (stop_) 
+        if (stop_)
         {
             qDebug("user requested receiver stop\n");
             break;
@@ -1031,9 +1028,8 @@ _exit:
     state_ = kFinished;
 }
 
-void PcapPort::PortReceiver::start()
+void PcapPort::EmulationTransceiver::start()
 {
-    // FIXME: return error
     if (state_ == kRunning) {
         qWarning("Receive start requested but is already running!");
         return;
@@ -1046,7 +1042,7 @@ void PcapPort::PortReceiver::start()
         QThread::msleep(10);
 }
 
-void PcapPort::PortReceiver::stop()
+void PcapPort::EmulationTransceiver::stop()
 {
     if (state_ == kRunning) {
         stop_ = true;
@@ -1054,18 +1050,17 @@ void PcapPort::PortReceiver::stop()
             QThread::msleep(10);
     }
     else {
-        // FIXME: return error
         qWarning("Receive stop requested but is not running!");
         return;
     }
 }
 
-bool PcapPort::PortReceiver::isRunning()
+bool PcapPort::EmulationTransceiver::isRunning()
 {
     return (state_ == kRunning);
 }
 
-int PcapPort::PortReceiver::transmitPacket(PacketBuffer *pktBuf)
+int PcapPort::EmulationTransceiver::transmitPacket(PacketBuffer *pktBuf)
 {
     return pcap_sendpacket(handle_, pktBuf->data(), pktBuf->length());
 }

server/pcapport.h

@@ -225,12 +225,11 @@ protected:
         volatile State  state_;
     };
 
-    // FIXME: rename? not just a 'receiver' but also 'transmitter'!
+    class EmulationTransceiver: public QThread
-    class PortReceiver: public QThread
     {
     public:
-        PortReceiver(const char *device, DeviceManager *deviceManager);
+        EmulationTransceiver(const char *device, DeviceManager *deviceManager);
-        ~PortReceiver();
+        ~EmulationTransceiver();
         void run();
         void start();
         void stop();
@@ -238,7 +237,7 @@ protected:
         int transmitPacket(PacketBuffer *pktBuf);
 
     private:
-        enum State 
+        enum State
         {
             kNotStarted,
             kRunning,
@@ -260,7 +259,7 @@ protected:
 private:
     PortTransmitter *transmitter_;
     PortCapturer    *capturer_;
-    PortReceiver    *receiver_;
+    EmulationTransceiver *emulXcvr_;
 
     static pcap_if_t *deviceList_;
 };

test/emultest.py

@@ -23,7 +23,7 @@ use_defaults = False
 # initialize defaults - drone
 host_name = '127.0.0.1'
 tx_port_number = -1
-rx_port_number = -1 
+rx_port_number = -1
 
 if sys.platform == 'win32':
     tshark = r'C:\Program Files\Wireshark\tshark.exe'
@@ -94,7 +94,7 @@ try:
     sudo('sysctl -w net.ipv4.ip_forward=1')
 
     # connect to drone
-    log.info('connecting to drone(%s:%d)' 
+    log.info('connecting to drone(%s:%d)'
             % (drone.hostName(), drone.portNumber()))
     drone.connect()
 
@@ -115,7 +115,7 @@ try:
     print('---------')
     for port in port_config_list.port:
         print('%d.%s (%s)' % (port.port_id.id, port.name, port.description))
-        # use a vhost port as default tx/rx port 
+        # use a vhost port as default tx/rx port
         if ('vhost' in port.name or 'sun' in port.description.lower()):
             if tx_port_number < 0:
                 tx_port_number = port.port_id.id
@@ -180,7 +180,7 @@ try:
     drone.addDeviceGroup(rx_dgid_list)
 
     # ----------------------------------------------------------------- #
-    # create stream on tx port - each test case will modify and reuse 
+    # create stream on tx port - each test case will modify and reuse
     # this stream as per its needs
     # ----------------------------------------------------------------- #
 
@@ -203,7 +203,7 @@ try:
 
     # ================================================================= #
     # ----------------------------------------------------------------- #
-    #                            TEST CASES 
+    #                            TEST CASES
     # ----------------------------------------------------------------- #
     # ================================================================= #
 
@@ -280,10 +280,10 @@ try:
         ip = p.Extensions[ip4]
         ip.src_ip = 0x0a0a0165
         ip.src_ip_mode = Ip4.e_im_inc_host
-        ip.src_ip_count = num_devs 
+        ip.src_ip_count = num_devs
         ip.dst_ip = 0x0a0a0265
         ip.dst_ip_mode = Ip4.e_im_inc_host
-        ip.dst_ip_count = num_devs 
+        ip.dst_ip_count = num_devs
 
         s.protocol.add().protocol_id.id = ost_pb.Protocol.kUdpFieldNumber
         s.protocol.add().protocol_id.id = ost_pb.Protocol.kPayloadFieldNumber
@@ -443,31 +443,31 @@ try:
         for i in range(num_vlans):
             vlan_id = vlan_base+i
             vrf = 'v' + str(vlan_id)
-            vlan_rx_dev = dut_rx_port + '.' + str(vlan_id) 
+            vlan_rx_dev = dut_rx_port + '.' + str(vlan_id)
-            vlan_tx_dev = dut_tx_port + '.' + str(vlan_id) 
+            vlan_tx_dev = dut_tx_port + '.' + str(vlan_id)
 
-            sudo('ip netns add ' + vrf) 
+            sudo('ip netns add ' + vrf)
 
-            sudo('ip link add link ' + dut_rx_port 
+            sudo('ip link add link ' + dut_rx_port
                  + ' name ' + vlan_rx_dev
                  + ' type vlan id ' + str(vlan_id))
-            sudo('ip link set ' + vlan_rx_dev 
+            sudo('ip link set ' + vlan_rx_dev
-                    + ' netns ' + vrf) 
+                    + ' netns ' + vrf)
-            sudo('ip netns exec ' + vrf 
+            sudo('ip netns exec ' + vrf
                     + ' ip addr add 10.1.1.1/24'
                     + ' dev ' + vlan_rx_dev)
-            sudo('ip netns exec ' + vrf 
+            sudo('ip netns exec ' + vrf
                     + ' ip link set ' + vlan_rx_dev + ' up')
 
-            sudo('ip link add link ' + dut_tx_port 
+            sudo('ip link add link ' + dut_tx_port
                  + ' name ' + vlan_tx_dev
                  + ' type vlan id ' + str(vlan_id))
-            sudo('ip link set ' + vlan_tx_dev 
+            sudo('ip link set ' + vlan_tx_dev
-                    + ' netns ' + vrf) 
+                    + ' netns ' + vrf)
-            sudo('ip netns exec ' + vrf 
+            sudo('ip netns exec ' + vrf
                     + ' ip addr add 10.1.2.1/24'
                     + ' dev ' + vlan_tx_dev)
-            sudo('ip netns exec ' + vrf 
+            sudo('ip netns exec ' + vrf
                     + ' ip link set ' + vlan_tx_dev + ' up')
 
 
@@ -510,7 +510,7 @@ try:
 
         # configure the tx stream(s)
         # we need more than one stream, so delete old one
-        # and create as many as we need 
+        # and create as many as we need
         # FIXME: restore the single stream at end?
         log.info('deleting tx_stream %d' % stream_id.stream_id[0].id)
         drone.deleteStream(stream_id)
@@ -529,7 +529,7 @@ try:
             s = stream_cfg.stream.add()
             s.stream_id.id = stream_id.stream_id[i].id
             s.core.is_enabled = True
-            s.core.ordinal = i 
+            s.core.ordinal = i
             s.control.packets_per_sec = 10
             s.control.num_packets = num_devs
 
@@ -572,14 +572,14 @@ try:
         for i in range(num_vlans):
             vlan_id = vlan_base + i
             vrf = 'v' + str(vlan_id)
-            vlan_rx_dev = dut_rx_port + '.' + str(vlan_id) 
+            vlan_rx_dev = dut_rx_port + '.' + str(vlan_id)
-            vlan_tx_dev = dut_tx_port + '.' + str(vlan_id) 
+            vlan_tx_dev = dut_tx_port + '.' + str(vlan_id)
 
-            sudo('ip netns exec ' + vrf 
+            sudo('ip netns exec ' + vrf
                     + ' ip neigh flush dev ' + vlan_rx_dev)
-            sudo('ip netns exec ' + vrf 
+            sudo('ip netns exec ' + vrf
                     + ' ip neigh flush dev ' + vlan_tx_dev)
-            sudo('ip netns exec ' + vrf 
+            sudo('ip netns exec ' + vrf
                     + ' ip neigh show')
 
         drone.startCapture(rx_port)
@@ -618,13 +618,13 @@ try:
         # show arp on DUT
         for i in range(num_vlans):
             vrf = 'v' + str(vlan_base + i)
-            sudo('ip netns exec ' + vrf 
+            sudo('ip netns exec ' + vrf
                     + ' ip neigh show')
         # un-configure the DUT
         for i in range(num_vlans):
             vlan_id = vlan_base + i
             vrf = 'v' + str(vlan_id)
-            sudo('ip netns delete ' + vrf) 
+            sudo('ip netns delete ' + vrf)
         suite.test_end(passed)
 
     # TODO: