Fixed various warnings on various platforms #2679

oviano · 2023-03-03T16:20:25Z

Addresses most (but not all) of the warnings documented here:

Plus various other warnings I've encountered on different platforms.

srtcore/core.cpp

ethouris · 2023-03-03T16:38:01Z

srtcore/logging.h

@@ -166,14 +166,20 @@ struct SRT_API LogDispatcher

        // See Logger::Logger; we know this has normally 2 characters,
        // except !!FATAL!!, which has 9. Still less than 32.


I think you have completely ignored what is written in this comment.

Here you can safely use strncpy with length 10 without worrying about the terminating 0.

Yes, and what happens when someone updates Logger::Logger to change those hardcoded values and doesn't update this comment?

Better to code defensively, IMHO.

If I predicted this would happen, I'd better use std::string for this first part. This way you can also use string::copy for that part. I would have to review it a bit more to refax it.

Indeed std::string would be better here, but let's go the way @oviano proposes here then.

ethouris · 2023-03-03T16:41:50Z

srtcore/logging.h

+        else
+        {
+            strncpy(prefix, your_pfx, MAX_PREFIX_SIZE);
+            prefix[MAX_PREFIX_SIZE] = '\0';


We'd be much better off, if you simply take the length by strlen and then copy using memcpy. Note that this isn't any performance critical part and it's done once in the beginning of an application. This way you can also use this length to insert ':' after that. This strcat with one colon was actually a little stupid ;)

But in that path I thought we don't want the colon? The old code only added the colon when there was room for both your_pfx and logger_pfx which the proposed changes does using the _snprint/snprintf.

In that, yes, but you have applied the fix for both paths.

Ok, maybe there is some confusion here.

To clarify, the warning I was dealing with was _CRT_SECURE_NO_WARNINGS. This applies to both the original initial strcpy call and then optional 2 x strcat. All need replacing, to remove that warning.

The changes are intended to provide the same end result, but using string function calls that don't trigger the above warning when used.

To be honest, there are probably 10 different ways to achieve the same thing, but this seemed as concise as any.

ethouris · 2023-03-03T16:43:00Z

BTW. Note that Max is on vacation now, so it may take a bit more time to finalize it.

maxsharabayko · 2023-04-18T09:57:17Z

srtcore/logging.h

@@ -166,14 +166,20 @@ struct SRT_API LogDispatcher

        // See Logger::Logger; we know this has normally 2 characters,
        // except !!FATAL!!, which has 9. Still less than 32.


Indeed std::string would be better here, but let's go the way @oviano proposes here then.

testing/srt-test-live.cpp

maxsharabayko · 2023-04-20T07:32:23Z

Hey @oviano. Thanks for the PR!
I've noticed you are keeping two commits in this PR every time you make changes. If you'd like them to be rebase-merged to master, could you please change the commit message format to "[core] Commit message."?
Please see Commit Message Format.

If I misunderstood your intent, just let me know, I'll go ahead and squash-merge the PR to master.

oviano · 2023-04-20T07:35:04Z

No I just hadn’t got round to squashing them again (there used to be lots of commits) - feel free to squash and merge as you wish.

* [core] Fix crypto mode auto for listener sender (Haivision#2711). Co-authored-by: oviano <ovcollyer@mac.com> * [build] Upgraded CI: ubuntu to version 20.04 (Haivision#2682). * [docs] Added the link for registration in slack to the getting started table (Haivision#2721). * [core] Fixed FEC Emergency resize crash (Haivision#2717). Fixed minimum history condition. * [core] Fixed various compiler warnings on various platforms (Haivision#2679). * [core] Minor fix of variable shadowing. * [tests] Minor fix of variable shadowing. * [build] Add -Wshadow=local to CMake build flags. Supported since GCC 7.0. * [core] Correct remaining endianness issues Fixes the last two remaining test failures on big-endian. These operations were all already no-ops on little-endian, and unnecessarily byteswapped the IP addresses on big-endian. Closes: Haivision#2697 * [docs] Minor updates to AEAD docs plus changed v1.6.0 to 1.5.2 in some files * [build] Fix downversioning of _WIN32_WINNT (Haivision#2754). * [core] Fixed unhandled error in haicrypt (Haivision#2685). * [core] Use overlapped WSASendTo to avoid loss in UDP sending (Haivision#2632). * [core] Add volatile keyword to asm block in rdtsc (Haivision#2759). * [core] Fixed srctime from closing socket was mistakenly cleared * [core] Fixed group read-ready epoll events. * [core] Removed unused CUDTGroup::m_Positions. * [core] Perf improvement of group reading. * [core] Fixed RCV buffer initialization in Rendezvous. * [docs] Updating the explicit information for binding to IPv6 wildcard (Haivision#2765). * [tests] Added custom main with transparent parameters for tests (Haivision#2681). * [core] Fix memory leak when can't buffer a HS packet (Haivision#2757). * [core] Refactor CRcvQueue::storePkt(..) for better resource management (Haivision#2775). * [core] Fix hang up on not enough space in the RCV buffer (Haivision#2745). When there is space available in the receiving buffer after it is full, send an ack to allow the sender to resume transmission. Reschedule sending if ACK decreases the flight span after sending is congested. Co-authored-by: Maxim Sharabayko <maxlovic@gmail.com> * [core] fix tsbpd() may deadlock with processCtrlShutdown() * [core] Slightly optimize the RCV drop by message number (Haivision#2686). Some minor improvements of logs and comments. * [core] Rejection not undertaken in rendezvous after KMX failure (Haivision#2692). * [core] Fix: In rendezvous when processing resulted in ACCEPT it was still sending rejection * [core] Minor code clean up in CRateEstimator. * [core] Initialize ISN and PeerISN in CUDT. * [core] Drop unencrypted packets in GCM mode. * [apps] Fix the build for target without IP_ADD_SOURCE_MEMBERSHIP (Haivision#2779). * [core] Added maximum BW limit for retransmissions (Haivision#2714). * [API] SRT version raised to 1.5.3. * [apps] Fixed conditional IP_ADD_SOURCE_MEMBERSHIP in testmedia (Haivision#2780). * [core] Fixed SRT_ATTR_REQUIRES use. * [build] Added missing public header files in Windows binary installer (Haivision#2784). The header file access_control.h was added to the source tree at some point but was not added to the Windows installer. --------- Co-authored-by: Maxim Sharabayko <maxlovic@gmail.com> Co-authored-by: oviano <ovcollyer@mac.com> Co-authored-by: Sektor van Skijlen <ethouris@gmail.com> Co-authored-by: Maria Sharabayko <41019697+mbakholdina@users.noreply.github.com> Co-authored-by: Maxim Sharabayko <maxsharabayko@haivision.com> Co-authored-by: matoro <matoro@users.noreply.github.com> Co-authored-by: Maria Sharabayko <msharabayko@haivision.com> Co-authored-by: Steve Lhomme <robux4@ycbcr.xyz> Co-authored-by: Aaron Jencks <32805004+aaron-jencks@users.noreply.github.com> Co-authored-by: Guangqing Chen <hi@goushi.me> Co-authored-by: john <hondaxiao@tencent.com> Co-authored-by: yomnes0 <127947185+yomnes0@users.noreply.github.com> Co-authored-by: Mikołaj Małecki <mmalecki@haivision.com> Co-authored-by: Jose Santiago <jsantiago@haivision.com> Co-authored-by: Thierry Lelegard <lelegard@users.noreply.github.com>

oviano mentioned this pull request Mar 3, 2023

Any plans to eliminate the many compile warnings, in e.g. Windows build? #1646

Closed

ethouris reviewed Mar 3, 2023

View reviewed changes

srtcore/core.cpp Outdated Show resolved Hide resolved

ethouris reviewed Mar 3, 2023

View reviewed changes

oviano force-pushed the more-warning-fixes branch from 03bda3c to 3a6db96 Compare March 31, 2023 09:08

This comment was marked as off-topic.

Sign in to view

oviano force-pushed the more-warning-fixes branch 2 times, most recently from 9da3ef9 to 5492b38 Compare April 12, 2023 11:10

oviano changed the title ~~More warning fixes~~ Fixed various warnings on various platforms Apr 12, 2023

maxsharabayko added Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core labels Apr 18, 2023

maxsharabayko reviewed Apr 18, 2023

View reviewed changes

maxsharabayko added this to the v1.5.2 milestone Apr 18, 2023

oviano added 2 commits April 19, 2023 17:53

Fixed various warnings on various platforms

546599c

Use sizeof in strncpy[_s], instead of hardcoded value

30d9603

oviano force-pushed the more-warning-fixes branch from f6faf91 to 30d9603 Compare April 19, 2023 16:53

maxsharabayko merged commit 6fcff6d into Haivision:master Apr 21, 2023

oviano deleted the more-warning-fixes branch April 22, 2023 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed various warnings on various platforms #2679

Fixed various warnings on various platforms #2679

oviano commented Mar 3, 2023 •

edited

Loading

ethouris Mar 3, 2023

oviano Mar 3, 2023

ethouris Mar 3, 2023

maxsharabayko Apr 18, 2023

ethouris Mar 3, 2023

oviano Mar 3, 2023

ethouris Mar 3, 2023

oviano Mar 3, 2023 •

edited

Loading

ethouris commented Mar 3, 2023

This comment was marked as off-topic.

maxsharabayko Apr 18, 2023

maxsharabayko commented Apr 20, 2023 •

edited

Loading

oviano commented Apr 20, 2023

		@@ -166,14 +166,20 @@ struct SRT_API LogDispatcher

		// See Logger::Logger; we know this has normally 2 characters,
		// except !!FATAL!!, which has 9. Still less than 32.

Fixed various warnings on various platforms #2679

Fixed various warnings on various platforms #2679

Conversation

oviano commented Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oviano Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

ethouris commented Mar 3, 2023

This comment was marked as off-topic.

Choose a reason for hiding this comment

maxsharabayko commented Apr 20, 2023 • edited Loading

oviano commented Apr 20, 2023

oviano commented Mar 3, 2023 •

edited

Loading

oviano Mar 3, 2023 •

edited

Loading

maxsharabayko commented Apr 20, 2023 •

edited

Loading