From 3a5396c865d57a15d951d6bd52505d225c2e2331 Mon Sep 17 00:00:00 2001 From: Srivats P Date: Sat, 14 Nov 2015 17:06:43 +0530 Subject: [PATCH] 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 | 3 ++ server/device.cpp | 16 +++++----- server/devicemanager.cpp | 15 ++++++---- server/devicemanager.h | 4 +-- server/drone.pro | 2 +- server/myservice.cpp | 25 +++++----------- server/pcapport.cpp | 65 +++++++++++++++++++--------------------- server/pcapport.h | 11 ++++--- test/emultest.py | 58 +++++++++++++++++------------------ 9 files changed, 94 insertions(+), 105 deletions(-) diff --git a/server/abstractport.h b/server/abstractport.h index 281dd00..8f3c7e7 100644 --- a/server/abstractport.h +++ b/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: diff --git a/server/device.cpp b/server/device.cpp index a6c0f95..804bf0d 100644 --- a/server/device.cpp +++ b/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(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 diff --git a/server/devicemanager.cpp b/server/devicemanager.cpp index e55d68e..e4bf9ff 100644 --- a/server/devicemanager.cpp +++ b/server/devicemanager.cpp @@ -32,7 +32,8 @@ along with this program. If not, see 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(pktData + offset); diff --git a/server/devicemanager.h b/server/devicemanager.h index 1a9b857..8f2ca2f 100644 --- a/server/devicemanager.h +++ b/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 deviceGroupList_; QHash deviceList_; diff --git a/server/drone.pro b/server/drone.pro index 9ba5046..42f324e 100644 --- a/server/drone.pro +++ b/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.* diff --git a/server/myservice.cpp b/server/myservice.cpp index 0271dbe..2c5fc62 100644 --- a/server/myservice.cpp +++ b/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 - * streams/ports and devices independent? + * =================================================================== + * XXX: Streams and Devices are largely non-overlapping from a RPC + * 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: diff --git a/server/pcapport.cpp b/server/pcapport.cpp index 642b514..8b372d8 100644 --- a/server/pcapport.cpp +++ b/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 - qDebug("%s:can't set promiscuous mode, trying non-promisc", - qPrintable(device_)); - flags &= ~PCAP_OPENFLAG_PROMISCUOUS; - goto _retry; + notify("Unable to set promiscuous mode on <%s> - " + "device emulation will not work", qPrintable(device_)); + goto _exit; } #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 - qDebug("%s: Error opening port %s: %s\n", __FUNCTION__, - device_.toAscii().constData(), errbuf); + notify("Unable to open <%s> [%s] - device emulation will not work", + qPrintable(device_), 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()); } diff --git a/server/pcapport.h b/server/pcapport.h index 5ed0254..f0ccde0 100644 --- a/server/pcapport.h +++ b/server/pcapport.h @@ -225,12 +225,11 @@ protected: volatile State state_; }; - // FIXME: rename? not just a 'receiver' but also 'transmitter'! - class PortReceiver: public QThread + class EmulationTransceiver: public QThread { public: - PortReceiver(const char *device, DeviceManager *deviceManager); - ~PortReceiver(); + EmulationTransceiver(const char *device, DeviceManager *deviceManager); + ~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_; }; diff --git a/test/emultest.py b/test/emultest.py index 8851c55..4402f27 100644 --- a/test/emultest.py +++ b/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_tx_dev = dut_tx_port + '.' + str(vlan_id) + vlan_rx_dev = dut_rx_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 - + ' netns ' + vrf) - sudo('ip netns exec ' + vrf + sudo('ip link set ' + vlan_rx_dev + + ' netns ' + 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 - + ' netns ' + vrf) - sudo('ip netns exec ' + vrf + sudo('ip link set ' + vlan_tx_dev + + ' netns ' + 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_tx_dev = dut_tx_port + '.' + str(vlan_id) + vlan_rx_dev = dut_rx_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: