From 6d7e44ef99f730c86e2ee672c55e43752f483848 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Ma=C5=82ecki?= Date: Tue, 3 Sep 2024 15:11:18 +0200 Subject: [PATCH 1/2] [core] Some refactoring per clang-reported warnings --- .github/workflows/cxx11-win.yaml | 4 +-- srtcore/congctl.cpp | 2 +- srtcore/handshake.cpp | 2 +- srtcore/logging.h | 51 ++++++++++++++++---------------- srtcore/queue.h | 4 +++ srtcore/utilities.h | 21 ++----------- testing/srt-test-live.cpp | 2 +- testing/srt-test-multiplex.cpp | 8 ++--- testing/srt-test-relay.cpp | 4 +-- testing/testactivemedia.cpp | 16 ++++++++++ testing/testactivemedia.hpp | 20 ++----------- testing/testmedia.cpp | 9 +++--- testing/testmedia.hpp | 5 ++-- testing/testmediabase.hpp | 4 +-- 14 files changed, 70 insertions(+), 82 deletions(-) diff --git a/.github/workflows/cxx11-win.yaml b/.github/workflows/cxx11-win.yaml index 1a9e10a95..f1554053d 100644 --- a/.github/workflows/cxx11-win.yaml +++ b/.github/workflows/cxx11-win.yaml @@ -17,8 +17,8 @@ jobs: - name: configure run: | md _build && cd _build - cmake ../ -DENABLE_STDCXX_SYNC=ON -DENABLE_ENCRYPTION=OFF -DENABLE_UNITTESTS=ON -DENABLE_BONDING=ON + cmake ../ -DENABLE_STDCXX_SYNC=ON -DENABLE_ENCRYPTION=OFF -DENABLE_UNITTESTS=ON -DENABLE_BONDING=ON -DUSE_CXX_STD=c++11 - name: build - run: cd _build && cmake --build ./ --config Release + run: cd _build && cmake --build ./ --config Release --verbose - name: test run: cd _build && ctest -E "TestIPv6.v6_calls_v4|TestConnectionTimeout.BlockingLoop" --extra-verbose -C Release diff --git a/srtcore/congctl.cpp b/srtcore/congctl.cpp index b9265c046..85d4cda97 100644 --- a/srtcore/congctl.cpp +++ b/srtcore/congctl.cpp @@ -595,7 +595,7 @@ class FileCC : public SrtCongestionControlBase { m_dPktSndPeriod = m_dCWndSize / (m_parent->SRTT() + m_iRCInterval); HLOGC(cclog.Debug, log << "FileCC: CHKTIMER, SLOWSTART:OFF, sndperiod=" << m_dPktSndPeriod << "us AS wndsize/(RTT+RCIV) (wndsize=" - << setprecision(6) << m_dCWndSize << " RTT=" << m_parent->SRTT() << " RCIV=" << m_iRCInterval << ")"); + << m_dCWndSize << " RTT=" << m_parent->SRTT() << " RCIV=" << m_iRCInterval << ")"); } } else diff --git a/srtcore/handshake.cpp b/srtcore/handshake.cpp index f8f03c84d..c97b4e2a3 100644 --- a/srtcore/handshake.cpp +++ b/srtcore/handshake.cpp @@ -300,7 +300,7 @@ std::string srt::SrtFlagString(int32_t flags) #define LEN(arr) (sizeof (arr)/(sizeof ((arr)[0]))) std::string output; - static std::string namera[] = { "TSBPD-snd", "TSBPD-rcv", "haicrypt", "TLPktDrop", "NAKReport", "ReXmitFlag", "StreamAPI" }; + static std::string namera[] = { "TSBPD-snd", "TSBPD-rcv", "haicrypt", "TLPktDrop", "NAKReport", "ReXmitFlag", "StreamAPI", "FilterCapable" }; size_t i = 0; for (; i < LEN(namera); ++i) diff --git a/srtcore/logging.h b/srtcore/logging.h index 3f4efb286..85261c70c 100644 --- a/srtcore/logging.h +++ b/srtcore/logging.h @@ -54,7 +54,7 @@ written by { \ srt_logging::LogDispatcher::Proxy log(logdes); \ log.setloc(__FILE__, __LINE__, __FUNCTION__); \ - const srt_logging::LogDispatcher::Proxy& log_prox SRT_ATR_UNUSED = args; \ + { (void)(const srt_logging::LogDispatcher::Proxy&)(args); } \ } // LOGF uses printf-like style formatting. @@ -147,6 +147,7 @@ struct SRT_API LogDispatcher LogLevel::type level; static const size_t MAX_PREFIX_SIZE = 32; char prefix[MAX_PREFIX_SIZE+1]; + size_t prefix_len; LogConfig* src_config; bool isset(int flg) { return (src_config->flags & flg) != 0; } @@ -159,30 +160,30 @@ struct SRT_API LogDispatcher level(log_level), src_config(&config) { - // XXX stpcpy desired, but not enough portable - // Composing the exact prefix is not critical, so simply - // cut the prefix, if the length is exceeded - - // See Logger::Logger; we know this has normally 2 characters, - // except !!FATAL!!, which has 9. Still less than 32. - // If the size of the FA name together with severity exceeds the size, - // just skip the former. - if (logger_pfx && strlen(prefix) + strlen(logger_pfx) + 1 < MAX_PREFIX_SIZE) + size_t your_pfx_len = your_pfx ? strlen(your_pfx) : 0; + size_t logger_pfx_len = logger_pfx ? strlen(logger_pfx) : 0; + + if (logger_pfx && your_pfx_len + logger_pfx_len + 1 < MAX_PREFIX_SIZE) { -#if defined(_MSC_VER) && _MSC_VER < 1900 - _snprintf(prefix, MAX_PREFIX_SIZE, "%s:%s", your_pfx, logger_pfx); -#else - snprintf(prefix, MAX_PREFIX_SIZE + 1, "%s:%s", your_pfx, logger_pfx); -#endif + memcpy(prefix, your_pfx, your_pfx_len); + prefix[your_pfx_len] = ':'; + memcpy(prefix + your_pfx_len + 1, logger_pfx, logger_pfx_len); + prefix[your_pfx_len + logger_pfx_len + 1] = '\0'; + prefix_len = your_pfx_len + logger_pfx_len + 1; + } + else if (your_pfx) + { + // Prefix too long, so copy only your_pfx and only + // as much as it fits + size_t copylen = std::min(+MAX_PREFIX_SIZE, your_pfx_len); + memcpy(prefix, your_pfx, copylen); + prefix[copylen] = '\0'; + prefix_len = copylen; } else { -#ifdef _MSC_VER - strncpy_s(prefix, MAX_PREFIX_SIZE + 1, your_pfx, _TRUNCATE); -#else - strncpy(prefix, your_pfx, MAX_PREFIX_SIZE); - prefix[MAX_PREFIX_SIZE] = '\0'; -#endif + prefix[0] = '\0'; + prefix_len = 0; } } @@ -338,9 +339,9 @@ struct LogDispatcher::Proxy ~Proxy() { - if ( that_enabled ) + if (that_enabled) { - if ( (flags & SRT_LOGF_DISABLE_EOL) == 0 ) + if ((flags & SRT_LOGF_DISABLE_EOL) == 0) os << std::endl; that.SendLogLine(i_file, i_line, area, os.str()); } @@ -381,7 +382,7 @@ struct LogDispatcher::Proxy buf[len-1] = '\0'; } - os << buf; + os.write(buf, len); return *this; } }; @@ -494,7 +495,7 @@ inline void LogDispatcher::SendLogLine(const char* file, int line, const std::st } else if ( src_config->log_stream ) { - (*src_config->log_stream) << msg; + src_config->log_stream->write(msg.data(), msg.size()); (*src_config->log_stream).flush(); } src_config->unlock(); diff --git a/srtcore/queue.h b/srtcore/queue.h index 132b670b3..48bedd9af 100644 --- a/srtcore/queue.h +++ b/srtcore/queue.h @@ -591,6 +591,10 @@ struct CMultiplexer , m_pRcvQueue(NULL) , m_pChannel(NULL) , m_pTimer(NULL) + , m_iPort(0) + , m_iIPversion(0) + , m_iRefCount(1) + , m_iID(-1) { } diff --git a/srtcore/utilities.h b/srtcore/utilities.h index 1786cf0ae..ca8c365a3 100644 --- a/srtcore/utilities.h +++ b/srtcore/utilities.h @@ -981,30 +981,13 @@ inline std::string FormatBinaryString(const uint8_t* bytes, size_t size) if ( size == 0 ) return ""; - //char buf[256]; using namespace std; ostringstream os; + os << setfill('0') << setw(2) << hex << uppercase; - // I know, it's funny to use sprintf and ostringstream simultaneously, - // but " %02X" in iostream is: << " " << hex << uppercase << setw(2) << setfill('0') << VALUE << setw(1) - // Too noisy. OTOH ostringstream solves the problem of memory allocation - // for a string of unpredictable size. - //sprintf(buf, "%02X", int(bytes[0])); - - os.fill('0'); - os.width(2); - os.setf(ios::basefield, ios::hex); - os.setf(ios::uppercase); - - //os << buf; - os << int(bytes[0]); - - - for (size_t i = 1; i < size; ++i) + for (size_t i = 0; i < size; ++i) { - //sprintf(buf, " %02X", int(bytes[i])); - //os << buf; os << int(bytes[i]); } return os.str(); diff --git a/testing/srt-test-live.cpp b/testing/srt-test-live.cpp index 1811220c8..12478e461 100644 --- a/testing/srt-test-live.cpp +++ b/testing/srt-test-live.cpp @@ -301,7 +301,7 @@ extern "C" int SrtCheckGroupHook(void* , SRTSOCKET acpsock, int , const sockaddr size = sizeof gt; if (-1 != srt_getsockflag(acpsock, SRTO_GROUPTYPE, >, &size)) { - if (gt < Size(gtypes)) + if (size_t(gt) < Size(gtypes)) Verb(" type=", gtypes[gt], VerbNoEOL); else Verb(" type=", int(gt), VerbNoEOL); diff --git a/testing/srt-test-multiplex.cpp b/testing/srt-test-multiplex.cpp index deb36554c..857557a5c 100644 --- a/testing/srt-test-multiplex.cpp +++ b/testing/srt-test-multiplex.cpp @@ -76,7 +76,7 @@ struct MediumPair bytevector initial_portion; string name; - MediumPair(unique_ptr s, unique_ptr t): src(move(s)), tar(move(t)) {} + MediumPair(unique_ptr s, unique_ptr t): src(std::move(s)), tar(std::move(t)) {} void Stop() { @@ -190,9 +190,9 @@ class MediaBase /// are still meant to be delivered to @c tar MediumPair& Link(std::unique_ptr src, std::unique_ptr tar, bytevector&& initial_portion, string name, string thread_name) { - media.emplace_back(move(src), move(tar)); + media.emplace_back(std::move(src), std::move(tar)); MediumPair& med = media.back(); - med.initial_portion = move(initial_portion); + med.initial_portion = std::move(initial_portion); med.name = name; // Ok, got this, so we can start transmission. @@ -382,7 +382,7 @@ bool SelectAndLink(SrtModel& m, string id, bool mode_output, string& w_msg) } bytevector dummy_initial_portion; - g_media_base.Link(move(source), move(target), move(dummy_initial_portion), os.str(), thread_name); + g_media_base.Link(std::move(source), std::move(target), std::move(dummy_initial_portion), os.str(), thread_name); return true; } diff --git a/testing/srt-test-relay.cpp b/testing/srt-test-relay.cpp index e7e5ae574..912af555e 100755 --- a/testing/srt-test-relay.cpp +++ b/testing/srt-test-relay.cpp @@ -320,7 +320,7 @@ SrtMainLoop::SrtMainLoop(const string& srt_uri, bool input_echoback, const strin Verb() << "Setting up output: " << spec; unique_ptr m { new TargetMedium }; m->Setup(Target::Create(spec)); - m_output_media.push_back(move(m)); + m_output_media.push_back(std::move(m)); } @@ -369,7 +369,7 @@ SrtMainLoop::SrtMainLoop(const string& srt_uri, bool input_echoback, const strin // Add SRT medium to output targets, and keep input medium empty. unique_ptr med { new TargetMedium }; med->Setup(m_srt_relay.get()); - m_output_media.push_back(move(med)); + m_output_media.push_back(std::move(med)); } else { diff --git a/testing/testactivemedia.cpp b/testing/testactivemedia.cpp index 96344f0b2..dd0393ea6 100644 --- a/testing/testactivemedia.cpp +++ b/testing/testactivemedia.cpp @@ -120,4 +120,20 @@ void TargetMedium::Runner() } } +bool TargetMedium::Schedule(const MediaPacket& data) +{ + LOGP(applog.Debug, "TargetMedium::Schedule LOCK ... "); + std::lock_guard lg(buffer_lock); + LOGP(applog.Debug, "TargetMedium::Schedule LOCKED - checking: running=", running, " interrupt=", ::transmit_int_state); + if (!running || ::transmit_int_state) + { + LOGP(applog.Debug, "TargetMedium::Schedule: not running, discarding packet"); + return false; + } + + LOGP(applog.Debug, "TargetMedium(", typeid(*med).name(), "): Schedule: [", data.payload.size(), "] CLIENT -> BUFFER"); + buffer.push_back(data); + ready.notify_one(); + return true; +} diff --git a/testing/testactivemedia.hpp b/testing/testactivemedia.hpp index 011dcbfe7..e92abbe0c 100644 --- a/testing/testactivemedia.hpp +++ b/testing/testactivemedia.hpp @@ -6,7 +6,6 @@ #include #include #include -#include #include #include "testmedia.hpp" @@ -29,7 +28,7 @@ struct Medium std::mutex buffer_lock; std::thread thr; std::condition_variable ready; - std::atomic running = {false}; + srt::sync::atomic running {false}; std::exception_ptr xp; // To catch exception thrown by a thread virtual void Runner() = 0; @@ -147,22 +146,7 @@ struct TargetMedium: Medium { void Runner() override; - bool Schedule(const MediaPacket& data) - { - LOGP(applog.Debug, "TargetMedium::Schedule LOCK ... "); - std::lock_guard lg(buffer_lock); - LOGP(applog.Debug, "TargetMedium::Schedule LOCKED - checking: running=", running, " interrupt=", ::transmit_int_state); - if (!running || ::transmit_int_state) - { - LOGP(applog.Debug, "TargetMedium::Schedule: not running, discarding packet"); - return false; - } - - LOGP(applog.Debug, "TargetMedium(", typeid(*med).name(), "): Schedule: [", data.payload.size(), "] CLIENT -> BUFFER"); - buffer.push_back(data); - ready.notify_one(); - return true; - } + bool Schedule(const MediaPacket& data); void Clear() { diff --git a/testing/testmedia.cpp b/testing/testmedia.cpp index b9d8a0413..a7eb0541c 100755 --- a/testing/testmedia.cpp +++ b/testing/testmedia.cpp @@ -20,7 +20,6 @@ #include #include #include -#include #include #if !defined(_WIN32) #include @@ -50,8 +49,8 @@ using srt_logging::SockStatusStr; using srt_logging::MemberStatusStr; #endif -std::atomic transmit_throw_on_interrupt {false}; -std::atomic transmit_int_state {false}; +srt::sync::atomic transmit_throw_on_interrupt {false}; +srt::sync::atomic transmit_int_state {false}; int transmit_bw_report = 0; unsigned transmit_stats_report = 0; size_t transmit_chunk_size = SRT_LIVE_DEF_PLSIZE; @@ -348,7 +347,7 @@ void SrtCommon::InitParameters(string host, string path, map par) } cc.token = token++; - m_group_nodes.push_back(move(cc)); + m_group_nodes.push_back(std::move(cc)); } par.erase("type"); @@ -3042,7 +3041,7 @@ extern unique_ptr CreateMedium(const string& uri) } if (ptr) - ptr->uri = move(u); + ptr->uri = std::move(u); return ptr; } diff --git a/testing/testmedia.hpp b/testing/testmedia.hpp index be72471d1..470e825ef 100644 --- a/testing/testmedia.hpp +++ b/testing/testmedia.hpp @@ -15,7 +15,8 @@ #include #include #include -#include + +#include // use srt::sync::atomic instead of std::atomic for the sake of logging #include "apputil.hpp" #include "statswriter.hpp" @@ -25,7 +26,7 @@ extern srt_listen_callback_fn* transmit_accept_hook_fn; extern void* transmit_accept_hook_op; -extern std::atomic transmit_int_state; +extern srt::sync::atomic transmit_int_state; extern std::shared_ptr transmit_stats_writer; diff --git a/testing/testmediabase.hpp b/testing/testmediabase.hpp index 04a85d435..686198787 100644 --- a/testing/testmediabase.hpp +++ b/testing/testmediabase.hpp @@ -11,17 +11,17 @@ #ifndef INC_SRT_COMMON_TRANMITBASE_HPP #define INC_SRT_COMMON_TRANMITBASE_HPP -#include #include #include #include #include #include +#include "sync.h" #include "uriparser.hpp" typedef std::vector bytevector; -extern std::atomic transmit_throw_on_interrupt; +extern srt::sync::atomic transmit_throw_on_interrupt; extern int transmit_bw_report; extern unsigned transmit_stats_report; extern size_t transmit_chunk_size; From 5d4227dd5ae55815853402f1b1ff4fd18c067d3b Mon Sep 17 00:00:00 2001 From: Sektor van Skijlen Date: Tue, 3 Sep 2024 16:40:26 +0200 Subject: [PATCH 2/2] Apply suggestions from code review Added suggestions from code review. Co-authored-by: Maxim Sharabayko --- srtcore/logging.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/srtcore/logging.h b/srtcore/logging.h index 85261c70c..7782245a2 100644 --- a/srtcore/logging.h +++ b/srtcore/logging.h @@ -160,8 +160,8 @@ struct SRT_API LogDispatcher level(log_level), src_config(&config) { - size_t your_pfx_len = your_pfx ? strlen(your_pfx) : 0; - size_t logger_pfx_len = logger_pfx ? strlen(logger_pfx) : 0; + const size_t your_pfx_len = your_pfx ? strlen(your_pfx) : 0; + const size_t logger_pfx_len = logger_pfx ? strlen(logger_pfx) : 0; if (logger_pfx && your_pfx_len + logger_pfx_len + 1 < MAX_PREFIX_SIZE) {