From bb0f1e36aa0bcc6ab6b23bb6b4b2eb26d09471e0 Mon Sep 17 00:00:00 2001 From: Adrien Guinet Date: Tue, 26 Feb 2019 10:52:08 +0100 Subject: [PATCH] Cleanup buses map container, to really contains the UsbBus object Issues that existed in 2012 do not exist anymore! --- CMakeLists.txt | 1 - include/usbtop/buses.h | 16 +----- include/usbtop/usb_stats.h | 25 ++++----- src/CMakeLists.txt | 2 +- src/buses.cpp | 108 ++++++++++++++++++------------------- src/main.cpp | 1 + src/usb_stats.cpp | 48 ++++++++--------- 7 files changed, 90 insertions(+), 111 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 675a961..8bcf093 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -8,7 +8,6 @@ set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake) # PCAP find_package(PCAP REQUIRED) -find_package(Boost REQUIRED COMPONENTS thread) include_directories(include) diff --git a/include/usbtop/buses.h b/include/usbtop/buses.h index 2782f96..6d83b08 100644 --- a/include/usbtop/buses.h +++ b/include/usbtop/buses.h @@ -38,10 +38,7 @@ namespace usbtop { class UsbBuses { typedef void(*bus_func_t)(UsbBus::id_type bus_id, const char* name, const char* desc); - typedef std::map list_buses_t; - -public: - ~UsbBuses(); + typedef std::map list_buses_t; public: static void list(bus_func_t f, const char* filter); @@ -53,7 +50,7 @@ class UsbBuses populate(); list_buses_t::iterator it; for (it = _buses.begin(); it != _buses.end(); it++) { - UsbBus* bus = it->second; + UsbBus* bus = &it->second; if (!filter || (filter && bus->name() == filter)) { nfiltered++; f(bus); @@ -73,15 +70,6 @@ class UsbBuses static bool _populated; private: - // AG: ideally, we shouldn't store a pointer into this std::map, - // because "emplace" can be use to create non-copyable object into - // a container. - // Two issues here: std::map::emplace isn't implemented in libstdc++ - // (as of August 2012), and, even with std::unordered_map, the only - // supported syntax is : - // map.emplace(key, UsbBus(...)), which involves a copy of the UsbBus object... (really usefull). - // The best would be to do: - // map.emplace(key, args_for_usb_bus) static list_buses_t _buses; }; diff --git a/include/usbtop/usb_stats.h b/include/usbtop/usb_stats.h index 9e7fc19..967c5ee 100644 --- a/include/usbtop/usb_stats.h +++ b/include/usbtop/usb_stats.h @@ -35,8 +35,6 @@ #include #include -#include -#include #define LIVE_SAMPLE_COUNT 128 @@ -48,10 +46,10 @@ class Stats: boost::noncopyable public: Stats(); Stats(Stats&& o): - _nbytes(o._nbytes), - _tN(o._tN), - _nsamples(o._nsamples), - _inst_data(std::move(o._inst_data)) + nbytes_(o.nbytes_), + tN_(o.tN_), + nsamples_(o.nsamples_), + inst_data_(std::move(o.inst_data_)) { } public: @@ -64,22 +62,21 @@ class Stats: boost::noncopyable private: // Global stats - size_t _nbytes; - double _tN; - size_t _nsamples; + size_t nbytes_; + double tN_; + size_t nsamples_; // "Instantaneous" stats - boost::circular_buffer _inst_data; - mutable boost::shared_mutex _access; + boost::circular_buffer inst_data_; // Timestamp when the application is launched. Used as t0 - static double _t0; + static double t0_; // Last "instantaneous" stats - double _last_inst_bw; + double last_inst_bw_; // Time window for statistics in seconds - double _stats_window; + double stats_window_; }; class UsbStats: boost::noncopyable diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 861c36c..d7f939c 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -14,6 +14,6 @@ usb_stats.cpp ) add_executable(usbtop ${SRC_FILES}) -target_link_libraries(usbtop ${PCAP_LIBRARIES} Boost::thread) +target_link_libraries(usbtop ${PCAP_LIBRARIES} pthread) install(TARGETS usbtop DESTINATION sbin) diff --git a/src/buses.cpp b/src/buses.cpp index 81b7595..9c6f536 100644 --- a/src/buses.cpp +++ b/src/buses.cpp @@ -40,88 +40,82 @@ #define USB_DEVICE_START "usbmon" -static size_t g_len_usb_dev_start = 5; // strlen(USB_DEVICE_START +static size_t g_len_usb_dev_start = strlen(USB_DEVICE_START); -std::map usbtop::UsbBuses::_buses; +std::map usbtop::UsbBuses::_buses; bool usbtop::UsbBuses::_populated = false; -usbtop::UsbBuses::~UsbBuses() -{ - list_buses_t::const_iterator it; - for (it = _buses.begin(); it != _buses.end(); it++) { - delete it->second; - } -} - void usbtop::UsbBuses::list(bus_func_t f, const char* filter) { - char errbuf[PCAP_ERRBUF_SIZE]; - pcap_if_t* list_devs; - if (pcap_findalldevs(&list_devs, errbuf) < 0) { - std::cerr << "Unable to list available devices: " << errbuf << std::endl; - pcap_freealldevs(list_devs); - exit(1); - } - - pcap_if_t* cur_dev = list_devs; - while (cur_dev) { - if (cur_dev->name) { - if ((strlen(cur_dev->name) > g_len_usb_dev_start) && - (!filter || (filter && strcmp(cur_dev->name, filter) == 0)) && - (memcmp(cur_dev->name, USB_DEVICE_START, g_len_usb_dev_start) == 0)) { - size_t bus_id = atoll(&cur_dev->name[g_len_usb_dev_start+1]); - f(bus_id, cur_dev->name, cur_dev->description); - } - } - cur_dev = cur_dev->next; - } - - pcap_freealldevs(list_devs); + char errbuf[PCAP_ERRBUF_SIZE]; + pcap_if_t* list_devs; + if (pcap_findalldevs(&list_devs, errbuf) < 0) { + std::cerr << "Unable to list available devices: " << errbuf << std::endl; + pcap_freealldevs(list_devs); + exit(1); + } + + pcap_if_t* cur_dev = list_devs; + while (cur_dev) { + if (cur_dev->name) { + if ((strlen(cur_dev->name) > g_len_usb_dev_start) && + (!filter || (filter && strcmp(cur_dev->name, filter) == 0)) && + (memcmp(cur_dev->name, USB_DEVICE_START, g_len_usb_dev_start) == 0)) { + size_t bus_id = atoll(&cur_dev->name[g_len_usb_dev_start]); + f(bus_id, cur_dev->name, cur_dev->description); + } + } + cur_dev = cur_dev->next; + } + + pcap_freealldevs(list_devs); } void usbtop::UsbBuses::show(const char* filter) { - printf("Name\t\tDescription\n"); - printf("---------------------------\n"); + printf("Name\t\tDescription\n"); + printf("---------------------------\n"); list([](UsbBus::id_type /*bus_id*/, const char* name, const char* desc) - { - printf("%s", name); - if (desc) { - printf("\t\t%s", desc); - } - printf("\n"); - }, filter); + { + printf("%s", name); + if (desc) { + printf("\t\t%s", desc); + } + printf("\n"); + }, filter); } void usbtop::UsbBuses::populate(const char* filter) { - if (_populated) { - return; - } - - list([](UsbBus::id_type bus_id, const char* name, const char* desc) - { - add_bus(bus_id, name, desc); - }, filter); - _populated = true; + if (_populated) { + return; + } + + list([](UsbBus::id_type bus_id, const char* name, const char* desc) + { + add_bus(bus_id, name, desc); + }, filter); + _populated = true; } void usbtop::UsbBuses::add_bus(UsbBus::id_type bus_id, const char* name, const char* desc) { - _buses.insert(std::make_pair(bus_id, new UsbBus(bus_id, name, desc))); + _buses.emplace(std::piecewise_construct, + std::forward_as_tuple(bus_id), + std::forward_as_tuple(bus_id, name, desc)); } usbtop::UsbBus* usbtop::UsbBuses::get_bus(UsbBus::id_type bus_id) { - list_buses_t::iterator it = _buses.find(bus_id); - if (it == _buses.end()) { - return NULL; - } + list_buses_t::iterator it = _buses.find(bus_id); + if (it == _buses.end()) { + return NULL; + } - return it->second; + return &it->second; } size_t usbtop::UsbBuses::size() { - return _buses.size(); + return _buses.size(); } diff --git a/src/main.cpp b/src/main.cpp index c37f170..26af553 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -37,6 +37,7 @@ #include #include #include +#include #include diff --git a/src/usb_stats.cpp b/src/usb_stats.cpp index e7b2a67..1a1a197 100644 --- a/src/usb_stats.cpp +++ b/src/usb_stats.cpp @@ -36,44 +36,44 @@ // TODO: // * we need to do some real tests & math here to have accurate values.. ! -double usbtop::Stats::_t0 = 0; +double usbtop::Stats::t0_ = 0; usbtop::Stats::Stats(): - _nbytes(0), - _tN(0), - _nsamples(0), - _inst_data(LIVE_SAMPLE_COUNT), - _last_inst_bw(0.0), - _stats_window(1.0) + nbytes_(0), + tN_(0), + nsamples_(0), + inst_data_(LIVE_SAMPLE_COUNT), + last_inst_bw_(0.0), + stats_window_(1.0) { } void usbtop::Stats::init() { - _t0 = usbtop::tools::get_current_timestamp(); + t0_ = usbtop::tools::get_current_timestamp(); } void usbtop::Stats::push(double timestamp, size_t spacket) { - _nsamples++; - _nbytes += spacket; + nsamples_++; + nbytes_ += spacket; - double first_ts = timestamp-_stats_window; + double first_ts = timestamp-stats_window_; // Remove oldest samples - _inst_data.push_back(sample_t(timestamp, spacket)); + inst_data_.push_back(sample_t(timestamp, spacket)); - if (timestamp < _tN+0.2) { + if (timestamp < tN_+0.2) { return; } - _tN = timestamp; + tN_ = timestamp; boost::circular_buffer::iterator it; boost::circular_buffer::iterator it_last_rem; bool to_rem = false; - for (it = _inst_data.begin(); it != _inst_data.end(); it++) { + for (it = inst_data_.begin(); it != inst_data_.end(); it++) { if (it->first >= first_ts) { break; } @@ -82,33 +82,33 @@ void usbtop::Stats::push(double timestamp, size_t spacket) } if (to_rem) { it_last_rem++; - _inst_data.erase(_inst_data.begin(), it_last_rem); + inst_data_.erase(inst_data_.begin(), it_last_rem); } // Compute instant bw at this instant size_t tsize = 0.0; { boost::circular_buffer::const_iterator it; - for (it = _inst_data.begin(); it != _inst_data.end(); it++) { + for (it = inst_data_.begin(); it != inst_data_.end(); it++) { tsize += it->second; } } - const double first_ts_buf = _inst_data.front().first; + const double first_ts_buf = inst_data_.front().first; if (timestamp == first_ts_buf) { - _last_inst_bw = 0.0; + last_inst_bw_ = 0.0; } else { - _last_inst_bw = ((double)tsize)/(timestamp-first_ts_buf); + last_inst_bw_ = ((double)tsize)/(timestamp-first_ts_buf); } } double usbtop::Stats::bw_instant() const { double last_ts_packet, last_inst_bw; - last_ts_packet = _tN; - last_inst_bw = _last_inst_bw; + last_ts_packet = tN_; + last_inst_bw = last_inst_bw_; double cur_ts = tools::get_current_timestamp(); - if (cur_ts >= last_ts_packet+_stats_window) { + if (cur_ts >= last_ts_packet+stats_window_) { // No packet in current window. Returns 0 return 0.0; } @@ -118,5 +118,5 @@ double usbtop::Stats::bw_instant() const double usbtop::Stats::bw_mean() const { - return (double)_nbytes/(_tN-_t0); + return (double)nbytes_/(tN_-t0_); }