From a8ac77f4706b7e40887426c446ebded4555885e4 Mon Sep 17 00:00:00 2001 From: Neale Ranns Date: Wed, 19 Dec 2018 02:34:59 -0800 Subject: [PATCH] VOM: stats fixes - double free of m_stat_data_seg - cleanup typedefs Change-Id: I6aeb070471b6c5c97ac4379d01bd242136f80091 Signed-off-by: Neale Ranns --- extras/vom/vom/interface.cpp | 2 +- extras/vom/vom/interface.hpp | 3 +- extras/vom/vom/stat_client.cpp | 72 +++++++++++++++++++++++------------------- extras/vom/vom/stat_client.hpp | 58 ++++++++++++++-------------------- extras/vom/vom/stat_reader.cpp | 25 ++++++++------- extras/vom/vom/types.hpp | 11 +++++++ 6 files changed, 88 insertions(+), 83 deletions(-) diff --git a/extras/vom/vom/interface.cpp b/extras/vom/vom/interface.cpp index 504511eabc9..97f7776ebc6 100644 --- a/extras/vom/vom/interface.cpp +++ b/extras/vom/vom/interface.cpp @@ -418,7 +418,7 @@ interface::set(const std::string& tag) } void -interface::set(counter_t count, const std::string& stat_type) +interface::set(const counter_t& count, const std::string& stat_type) { if ("rx" == stat_type) m_stats.m_rx = count; diff --git a/extras/vom/vom/interface.hpp b/extras/vom/vom/interface.hpp index 100c3caf4bc..2f6511eac6d 100644 --- a/extras/vom/vom/interface.hpp +++ b/extras/vom/vom/interface.hpp @@ -25,7 +25,6 @@ #include "vom/route_domain.hpp" #include "vom/rpc_cmd.hpp" #include "vom/singular_db.hpp" -#include "vom/stat_client.hpp" namespace VOM { /** @@ -619,7 +618,7 @@ private: /** * Set the interface stat */ - void set(counter_t count, const std::string& stat_type); + void set(const counter_t& count, const std::string& stat_type); /** * enable the interface stats in the singular instance diff --git a/extras/vom/vom/stat_client.cpp b/extras/vom/vom/stat_client.cpp index b348c6c24eb..6406e215193 100644 --- a/extras/vom/vom/stat_client.cpp +++ b/extras/vom/vom/stat_client.cpp @@ -17,29 +17,22 @@ namespace VOM { -stat_client::stat_data_t::stat_data_t() - : m_name("") - , m_type(STAT_DIR_TYPE_ILLEGAL) -{ -} - -stat_client::stat_data_t::stat_data_t(stat_segment_data_t* stat_seg_data) - : m_name(stat_seg_data->name) - , m_type(stat_seg_data->type) +stat_client::stat_data_t::stat_data_t(const stat_segment_data_t& stat_seg_data) + : m_name(stat_seg_data.name) + , m_type(stat_seg_data.type) { switch (m_type) { case STAT_DIR_TYPE_SCALAR_INDEX: - m_scalar_value = stat_seg_data->scalar_value; + m_scalar_value = stat_seg_data.scalar_value; break; case STAT_DIR_TYPE_COUNTER_VECTOR_SIMPLE: - m_simple_counter_vec = stat_seg_data->simple_counter_vec; + m_simple_counter_vec = stat_seg_data.simple_counter_vec; break; case STAT_DIR_TYPE_COUNTER_VECTOR_COMBINED: - m_combined_counter_vec = - reinterpret_cast(stat_seg_data->combined_counter_vec); + m_combined_counter_vec = stat_seg_data.combined_counter_vec; break; case STAT_DIR_TYPE_ERROR_INDEX: - m_error_Value = stat_seg_data->error_value; + m_error_value = stat_seg_data.error_value; break; case STAT_DIR_TYPE_ILLEGAL: break; @@ -59,22 +52,25 @@ stat_client::stat_data_t::type() const } double -stat_client::stat_data_t::get_stat_segment_scalar_data() +stat_client::stat_data_t::get_stat_segment_scalar_data() const { return m_scalar_value; } + uint64_t -stat_client::stat_data_t::get_stat_segment_error_data() +stat_client::stat_data_t::get_stat_segment_error_data() const { - return m_error_Value; + return m_error_value; } + uint64_t** -stat_client::stat_data_t::get_stat_segment_simple_counter_data() +stat_client::stat_data_t::get_stat_segment_simple_counter_data() const { return m_simple_counter_vec; } -counter_t** -stat_client::stat_data_t::get_stat_segment_combined_counter_data() + +vlib_counter_t** +stat_client::stat_data_t::get_stat_segment_combined_counter_data() const { return m_combined_counter_vec; } @@ -83,6 +79,9 @@ stat_client::stat_client(std::string& socket_name) : m_socket_name(socket_name) , m_patterns() , m_stat_connect(0) + , m_counter_vec() + , m_stat_seg_data(nullptr) + , m_stat_data() { m_patterns.push_back("/if"); } @@ -91,6 +90,9 @@ stat_client::stat_client(std::vector& pattern) : m_socket_name("/run/vpp/stats.sock") , m_patterns(pattern) , m_stat_connect(0) + , m_counter_vec() + , m_stat_seg_data(nullptr) + , m_stat_data() { } @@ -99,6 +101,9 @@ stat_client::stat_client(std::string socket_name, : m_socket_name(socket_name) , m_patterns(patterns) , m_stat_connect(0) + , m_counter_vec() + , m_stat_seg_data(nullptr) + , m_stat_data() { } @@ -106,6 +111,9 @@ stat_client::stat_client() : m_socket_name("/run/vpp/stats.sock") , m_patterns() , m_stat_connect(0) + , m_counter_vec() + , m_stat_seg_data(nullptr) + , m_stat_data() { m_patterns.push_back("/if"); } @@ -127,8 +135,10 @@ stat_client::stat_client(const stat_client& o) int stat_client::connect() { - if (stat_segment_connect(m_socket_name.c_str()) == 0) + if (stat_segment_connect(m_socket_name.c_str()) == 0) { m_stat_connect = 1; + ls(); + } return m_stat_connect; } @@ -165,19 +175,17 @@ stat_client::ls() const stat_client::stat_data_vec_t& stat_client::dump() { - stat_segment_data_t* ssd; stat_segment_data_free(m_stat_seg_data); if (m_stat_data.size()) { m_stat_data.clear(); } - ssd = stat_segment_dump(m_counter_vec); - if (!ssd) { + m_stat_seg_data = stat_segment_dump(m_counter_vec); + if (!m_stat_seg_data) { ls(); return m_stat_data; } - m_stat_seg_data = ssd; - for (int i = 0; i < stat_segment_vec_len(ssd); i++) { - stat_data_t sd(&ssd[i]); + for (int i = 0; i < stat_segment_vec_len(m_stat_seg_data); i++) { + stat_data_t sd(m_stat_seg_data[i]); m_stat_data.push_back(sd); } return m_stat_data; @@ -186,19 +194,17 @@ stat_client::dump() const stat_client::stat_data_vec_t& stat_client::dump_entry(uint32_t index) { - stat_segment_data_t* ssd; stat_segment_data_free(m_stat_seg_data); if (m_stat_data.size()) { m_stat_data.clear(); } - ssd = stat_segment_dump_entry(index); - if (!ssd) { + m_stat_seg_data = stat_segment_dump_entry(index); + if (!m_stat_seg_data) { ls(); return m_stat_data; } - m_stat_seg_data = ssd; - for (int i = 0; i < stat_segment_vec_len(ssd); i++) { - stat_data_t sd(&ssd[i]); + for (int i = 0; i < stat_segment_vec_len(m_stat_seg_data); i++) { + stat_data_t sd(m_stat_seg_data[i]); m_stat_data.push_back(sd); } return m_stat_data; diff --git a/extras/vom/vom/stat_client.hpp b/extras/vom/vom/stat_client.hpp index 4da968a8204..40729feef5f 100644 --- a/extras/vom/vom/stat_client.hpp +++ b/extras/vom/vom/stat_client.hpp @@ -26,13 +26,6 @@ extern "C" { namespace VOM { -typedef struct -{ - uint64_t packets; - uint64_t bytes; -} counter_t; - -typedef uint64_t stat_counter_t; /** * A representation of a stat client in VPP */ @@ -44,15 +37,10 @@ public: */ struct stat_data_t { - /** - * stat data constructor - */ - stat_data_t(); - /** * stat data custom constructor */ - stat_data_t(stat_segment_data_t* stat_seg_data); + stat_data_t(const stat_segment_data_t& stat_seg_data); /** * get name of stat @@ -67,10 +55,10 @@ public: /** * Get pointer to actual data */ - double get_stat_segment_scalar_data(); - uint64_t get_stat_segment_error_data(); - uint64_t** get_stat_segment_simple_counter_data(); - counter_t** get_stat_segment_combined_counter_data(); + double get_stat_segment_scalar_data() const; + uint64_t get_stat_segment_error_data() const; + uint64_t** get_stat_segment_simple_counter_data() const; + vlib_counter_t** get_stat_segment_combined_counter_data() const; private: /** @@ -89,9 +77,9 @@ public: union { double m_scalar_value; - uint64_t m_error_Value; - uint64_t** m_simple_counter_vec; - counter_t** m_combined_counter_vec; + uint64_t m_error_value; + counter_t** m_simple_counter_vec; + vlib_counter_t** m_combined_counter_vec; }; }; @@ -141,43 +129,43 @@ public: void disconnect(); /** - * Get vector length of VPP style vector + * dump all the stats for given pattern */ - int vec_len(void* vec); + const stat_data_vec_t& dump(); /** - * Free VPP style vector + * dump stats for given index in stat directory */ - void vec_free(void* vec); + const stat_data_vec_t& dump_entry(uint32_t index); /** - * ls on the stat directory using given pattern + * Get vector length of VPP style vector */ - void ls(); + int vec_len(void* vec); + + double heartbeat(); /** - * dump all the stats for given pattern + * get index to name of stat */ - const stat_data_vec_t& dump(); + std::string index_to_name(uint32_t index); +private: /** - * dump stats for given index in stat directory + * Free VPP style vector */ - const stat_data_vec_t& dump_entry(uint32_t index); + void vec_free(void* vec); /** * Free stat segment data */ void data_free(); - double heartbeat(); - /** - * get index to name of stat + * ls on the stat directory using given pattern */ - std::string index_to_name(uint32_t index); + void ls(); -private: /** * socket name */ diff --git a/extras/vom/vom/stat_reader.cpp b/extras/vom/vom/stat_reader.cpp index 931d0888f71..82e09aedce0 100644 --- a/extras/vom/vom/stat_reader.cpp +++ b/extras/vom/vom/stat_reader.cpp @@ -61,7 +61,7 @@ stat_reader::unregisters(const interface& intf) void stat_reader::read() { - stat_client::stat_data_vec_t sd = m_client.dump(); + const stat_client::stat_data_vec_t& sd = m_client.dump(); for (auto& sde : sd) { std::string name; @@ -74,28 +74,29 @@ stat_reader::read() case STAT_DIR_TYPE_COUNTER_VECTOR_SIMPLE: case STAT_DIR_TYPE_ERROR_INDEX: case STAT_DIR_TYPE_SCALAR_INDEX: + case STAT_DIR_TYPE_ILLEGAL: break; - case STAT_DIR_TYPE_COUNTER_VECTOR_COMBINED: + case STAT_DIR_TYPE_COUNTER_VECTOR_COMBINED: { + vlib_counter_t** data; + + data = sde.get_stat_segment_combined_counter_data(); + if (name.find("/if") != std::string::npos) name.erase(0, 4); for (auto& i : m_stat_itf_indexes) { - counter_t count = {.packets = 0, .bytes = 0 }; - for (int k = 0; k < m_client.vec_len( - sde.get_stat_segment_combined_counter_data()); - k++) { - count.packets += - sde.get_stat_segment_combined_counter_data()[k][i].packets; - count.bytes += - sde.get_stat_segment_combined_counter_data()[k][i].bytes; + counter_t count; + + for (int k = 0; k < m_client.vec_len(data); k++) { + count.packets += data[k][i].packets; + count.bytes += data[k][i].bytes; } std::shared_ptr itf = interface::find(i); if (itf) itf->set(count, name); } break; - - default:; + } } } diff --git a/extras/vom/vom/types.hpp b/extras/vom/vom/types.hpp index ab214085d16..e3b21add045 100644 --- a/extras/vom/vom/types.hpp +++ b/extras/vom/vom/types.hpp @@ -378,6 +378,17 @@ struct l2_address_t std::vector bytes; }; +struct counter_t +{ + counter_t() + : packets(0) + , bytes(0) + { + } + uint64_t packets; + uint64_t bytes; +}; + /** * Ostream operator for a MAC address */ -- 2.16.6