2
\$\begingroup\$

I'm developing a toy project for monitoring some network parameters. Now I'm implementing a feature that will allow to measure a packet round trip between hosts by measuring time between TCP SYNC and TCP SYNC/ACK. One of the options for feeding this application with packets will be setting up capture on network device. I implemented class 'PacketReceiver' which responsibility is:

  • Listening on given network interface
  • Dispatch packets for subscribed observers

Aim of the project is to learn by making mistakes and getting feedback, so each constructive critique is very welcomed.

My considerations:

  • Does the locking on every layer on every packet in DistributePacket function could/should be done better?
  • DistributePacket function iterates over every layer of packet and does find by protocol and then iterates over every observer. It will probably impact performance. How it could be optimized?
  • Is interface of the class appropriate? Maybe something should be decoupled according to best practices?
  • Is the code readable? Is there anything that could make it more readable?
  • Is there something that could be improved in terms of best programming practices?

PacketReceiver.cpp

#include <PcapLiveDevice.h>
#include <iostream>
#include <functional>
#include "PacketReceiver.h"
#include "TcpLayer.h"

void PacketReceiver::OnPacketArrived(pcpp::RawPacket* pPacket, pcpp::PcapLiveDevice* pDevice, void* userCookie){
    auto packet = std::make_shared<pcpp::Packet>(pPacket);
    auto packetReceiver = static_cast<PacketReceiver*>(userCookie);

    packetReceiver->m_packetQueue.Enqueue(std::move(packet));
}

bool PacketReceiver::StartCapturing() {
    auto& devices = pcpp::PcapLiveDeviceList::getInstance();
    auto deviceList = devices.getPcapLiveDevicesList();

    pcpp::PcapLiveDevice* wlp7s0 = devices.getPcapLiveDeviceByName(m_captureName);
    if(!wlp7s0){
        std::cout << "Device: wlp7s0 was not found." << std::endl;
        return false;
    }

    if(!wlp7s0->open()){
        std::cout << "Device wlp7s0 could not be opened." << std::endl;
        return false;
    }

    if(!wlp7s0->startCapture(OnPacketArrived, this)){
        std::cout << "Unable to start capturing." << std::endl;
        return false;
    }
}

bool PacketReceiver::AddFilter(PacketObserverShPtr observer, const pcpp::ProtocolType protocol) noexcept{

    auto attachedObserver = m_packetObservers.find(observer);
    if(attachedObserver==m_packetObservers.end()) {
        return false;
    }

    {
        std::unique_lock<std::mutex> lock(m_mutex);

        auto subscribedObservers = m_protocolToObserver[protocol];
        subscribedObservers.insert(observer);
    }

    return true;
}

bool PacketReceiver::RemoveFilter(PacketObserverShPtr observer, const pcpp::ProtocolType protocol)  noexcept{
    auto attachedObserver = m_packetObservers.find(observer);
    if(attachedObserver==m_packetObservers.end()) {
        return false;
    }

    auto subscribedObservers = m_protocolToObserver.find(protocol);
    if(subscribedObservers==m_protocolToObserver.end()) {
        return false;
    }

    auto protocolObserver = subscribedObservers->second.find(observer);
    if(protocolObserver!=subscribedObservers->second.end()){
        std::unique_lock<std::mutex> lock(m_mutex);
        subscribedObservers->second.erase(protocolObserver);
    }

    return true;
}

bool PacketReceiver::Attach(PacketObserverShPtr observer) noexcept{
    std::unique_lock<std::mutex> lock(m_mutex);

    auto result = m_packetObservers.insert(observer);
    if(!result.second)
        return false;

    return true;
}

bool PacketReceiver::Detach(PacketObserverShPtr observer) noexcept{
    auto searchResult = m_packetObservers.find(observer);
    if(searchResult==m_packetObservers.end()) {
        return  false;
    }

    {
        std::unique_lock<std::mutex> lock(m_mutex);

        m_packetObservers.erase(searchResult);
        for(auto& protocol: m_protocolToObserver){
            protocol.second.erase(observer);
        }
    }

    return true;
}

void PacketReceiver::MainLoop() {
    while(m_runThread){
        auto packet = m_packetQueue.Dequeue();
        DistributePacket(std::move(packet));
    }
}

void PacketReceiver::OnThreadStarting() {
    StartCapturing();
}

void PacketReceiver::DistributePacket(std::shared_ptr<pcpp::Packet> packet) {
for(auto currentLayer = packet->getFirstLayer(); currentLayer!=NULL; currentLayer=packet->getFirstLayer()){
    auto protocol = currentLayer->getProtocol();

    std::unique_lock<std::mutex> lock(m_mutex);
    auto observers = m_protocolToObserver.find(protocol);
    if(observers!=m_protocolToObserver.end()){
        for(auto& observer : observers->second){
            observer->Update(std::move(packet));
        }
    }
}
}

PacketReceiver::PacketReceiver(const std::string &captureName) : m_captureName(captureName) {

}

PacketReceiver.hpp

#pragma  once

#include <PcapLiveDeviceList.h>
#include <atomic>
#include <thread>
#include <zmq.hpp>
#include <set>
#include <map>
#include "PacketQueue.h"
#include "Thread.h"
#include "Interfaces.h"

class PacketReceiver : public Thread {
public:
    PacketReceiver(const std::string& captureName);

    using PacketObserverShPtr = std::shared_ptr<IObserver<std::shared_ptr<pcpp::Packet> > >;

    bool Attach(PacketObserverShPtr observer) noexcept;
    bool Detach(PacketObserverShPtr observer) noexcept;

    bool AddFilter(PacketObserverShPtr observer, const pcpp::ProtocolType protocol) noexcept;
    bool RemoveFilter(PacketObserverShPtr observer, const pcpp::ProtocolType protocol) noexcept;
private:
    void MainLoop() override;
    void OnThreadStarting() override;

    bool StartCapturing();
    static void OnPacketArrived(pcpp::RawPacket* pPacket, pcpp::PcapLiveDevice* pDevice, void* userCookie);
    void DistributePacket(std::shared_ptr<pcpp::Packet> packet);

    PacketQueue m_packetQueue;
    std::set<PacketObserverShPtr > m_packetObservers;
    std::map<pcpp::ProtocolType, std::set<PacketObserverShPtr> > m_protocolToObserver;
    std::mutex m_mutex;
    std::string m_captureName;
};
\$\endgroup\$
1
\$\begingroup\$

I will give you some tips related to packet processing:

  • Have mutex/lock for every read packet is a bad idea in terms of performance
  • Copy packets form one place to another is also a bad idea, you should move packets or keep reference the same part, take into consideration that memory copy operations are expensive for a packet application like yours.

In general your code is readable and is clear what you want to achieve, I will suggest you to use a profiler in order to find the bottlenecks.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.