Refactoring and debug improvements series from group code #1028

ethouris · 2019-12-19T12:34:31Z

NOTE: Treat this thing as a draft and orientation guide. Things submitted here will be split into single PRs:

Major reasons for changes here:

Short functions that simplify various activities written elaborated way before.
Added more debug logs and improved some existing ones.
Might be some minor bugfixes, catching situations that weren't the problem before, but may catch some IPEs or situations that have arisen only in the socket group code.
Extra API that will be required in the new group API - these are parts that are not directly group-related.

The code is added with extra comments in the review section.

…s comparison for Mac

…eck in SockaddrToString

ethouris · 2020-01-20T17:42:37Z

apps/srt-tunnel.cpp

@@ -91,7 +91,7 @@ class Medium
    template <class DerivedMedium, class SocketType>
    static Medium* CreateAcceptor(DerivedMedium* self, const sockaddr_in& sa, SocketType sock, size_t chunk)
    {
-        string addr = SockaddrToString((sockaddr*)&sa);
+        string addr = SockaddrToString(sockaddr_any((sockaddr*)&sa, sizeof sa));


This is a fix for the safer use of the sockaddr_any here (involves checking the source object size).

ethouris · 2020-01-20T17:44:02Z

srtcore/api.cpp

@@ -110,13 +98,63 @@ CUDTSocket::~CUDTSocket()
   pthread_cond_destroy(&m_AcceptCond);
 }

+


These are functions that will be later intermediately used in the socket group code. Some of them are not part of the group code, just they may use other functions that are.

ethouris · 2020-01-20T17:45:58Z

srtcore/api.cpp

@@ -247,6 +291,95 @@ int CUDTUnited::cleanup()
   return 0;
 }

+SRTSOCKET CUDTUnited::generateSocketID()


The generation of the socket ID is now widely changed because the socket ID will be also used for groups. Socket ID will now be using less bits as before, as one bit is now reserved to mark that the ID is of a group. New identifiers will come from the same pool, except that for the groups it will get this additional bit.

ethouris · 2020-01-20T17:46:35Z

srtcore/api.cpp

@@ -309,13 +437,15 @@ int CUDTUnited::newConnection(const SRTSOCKET listen, const sockaddr_any& peer,

   // Can't manage this error through an exception because this is
   // running in the listener loop.
-   CUDTSocket* ls = locate(listen);
+   CUDTSocket* ls = locateSocket(listen);


See definition for locateSocket.

ethouris · 2020-01-20T17:47:20Z

srtcore/api.cpp

@@ -507,13 +659,18 @@ int CUDTUnited::newConnection(const SRTSOCKET listen, const sockaddr_any& peer,
   return 1;
 }

+// static forwarder


This has been just moved out of the header file.

ethouris · 2020-01-20T17:48:01Z

srtcore/api.cpp

@@ -524,25 +681,12 @@ int CUDTUnited::installAcceptHook(const SRTSOCKET lsn, srt_listen_callback_fn* h
    return 0;
 }



Removed lookup : see definition of locateSocket.

ethouris · 2020-01-20T17:49:30Z

srtcore/api.cpp

@@ -782,19 +906,45 @@ SRTSOCKET CUDTUnited::accept(const SRTSOCKET listen, sockaddr* pw_addr, int* pw_
   return u;
 }

-int CUDTUnited::connect(const SRTSOCKET u, const sockaddr* name, int namelen, int32_t forced_isn)
+int CUDTUnited::connect(SRTSOCKET u, const sockaddr* srcname, int srclen, const sockaddr* tarname, int tarlen)


This is an extra version of connect that maps to srt_connect_bind. The latter is used as a combination of bind and connect (simple replacement in case of sockets). However this bind-then-connect cannot be used this way for the socket groups, therefore it needs an extra function for it.

ethouris · 2020-01-20T17:50:48Z

srtcore/api.cpp

@@ -926,7 +1080,7 @@ int CUDTUnited::close(const SRTSOCKET u)
       CTimer::triggerEvent();
   }

-   HLOGC(mglog.Debug, log << "%" << u << ": GLOBAL: CLOSING DONE");
+   HLOGC(mglog.Debug, log << "@" << u << ": GLOBAL: CLOSING DONE");


You'll find more replacement from % into @ in the logs. This was a mistake earlier to use the % as a number prefix here. The % is meant to be a prefix for a sequence number, while @ is to be a prefix for a socket id.

ethouris · 2020-01-20T17:53:11Z

srtcore/api.cpp

@@ -1356,16 +1499,20 @@ int CUDTUnited::epoll_release(const int eid)
   return m_EPoll.release(eid);
 }

-CUDTSocket* CUDTUnited::locate(const SRTSOCKET u)
+CUDTSocket* CUDTUnited::locateSocket(const SRTSOCKET u, ErrorHandling erh)


The reason why locate was renamed to locateSocket was first of all because there will be another function added, locateGroup.

Also the lookup function has been removed because it duplicates the functionality of locate and the only difference was the way of informing about the failure: locate returned error, lookup threw exception. These two things have been now made common by locateSocket which's error handling method is selected through the additional parameter.

ethouris · 2020-01-20T17:55:06Z

srtcore/api.cpp

@@ -1908,6 +2061,18 @@ SRTSOCKET CUDT::socket()
   }
 }

+int CUDT::setError(const CUDTException& e)


One of them has been moved here from the header file - due to the reason of solving the header dependency problem. The other one is useful for quick returning a failure.

ethouris · 2020-01-20T17:56:29Z

srtcore/api.cpp

       }
-       return s_UDTUnited.bind(u, sa);
+       CUDTSocket* s = s_UDTUnited.locateSocket(u);


The CUDTUnited::bind has been changed to get the CUDTSocket* instead of SRTSOCKET and the socket lookup part was moved out of this. This is because this function will get much wider use, including a situation when the CUDTSocket object is already at hand.

ethouris · 2020-01-20T17:58:49Z

srtcore/buffer.h

+// a +% b : shift a by b
+// a == b : equality is same as for just numbers
+
+


This time this convention IS used in some comments.

ethouris · 2020-01-20T17:59:13Z

srtcore/channel.cpp

@@ -61,18 +61,13 @@ modified by
 #include "packet.h"
 #include "api.h" // SockaddrToString - possibly move it to somewhere else
 #include "logging.h"
+#include "netinet_any.h"
 #include "utilities.h"

 #ifdef _WIN32
    typedef int socklen_t;
 #endif



This below is moved to common.h

ethouris · 2020-01-20T18:05:24Z

srtcore/core.cpp

@@ -5408,6 +5403,9 @@ bool CUDT::close()
        m_bConnected = false;
    }

+    if (m_pCryptoControl)
+        m_pCryptoControl->close();
+


This is a bugfix for something that has appeared only during testing of the group code. The problem was that sometimes the close call wasn't done when required, or sometimes it was attempted to be done twice (after the pointer was cleared or before it was set to a valid object).

ethouris · 2020-01-20T18:12:20Z

srtcore/core.cpp

                    self->m_pRcvBuffer->skipData(seqlen);

                    self->m_iRcvLastSkipAck = skiptoseqno;

 #if ENABLE_LOGGING
                    int64_t timediff_us = 0;
                    if (!is_zero(tsbpdtime))
-                        timediff_us = count_microseconds(tsbpdtime - steady_clock::now());
+                        timediff_us = count_microseconds(steady_clock::now() - tsbpdtime);


Debug value fix. This was displayed usually when tsbpdtime was already in the past, so it was practically always displaying a negative value.

ethouris · 2020-01-20T18:20:04Z

srtcore/core.cpp

                    num = m_pSndLossList->insert(m_iSndLastAck, losslist_hi);
                }
+                else
+                {


This is a change in the current code that reacts on a situation that wasn't handled before at all. This is a handler for a situation when the lossreport declares a range of packets that as a whole is located behind the current value of m_iSndLastAck, that is, refers to packets that have been already dismissed from the sender buffer, or weren't ever in the sender buffer at all. This kind of situation is virtually impossible to happen normally in SRT, however the group code introduce some code based on ISN screwup that adds a risk that this could happen.

ethouris · 2020-01-20T18:22:14Z

srtcore/core.cpp

-            LOGC(dlog.Error,
-                 log << "IPE: packLostData: LOST packet negative offset: seqoff(m_iSeqNo " << w_packet.m_iSeqNo
-                     << ", m_iSndLastDataAck " << m_iSndLastDataAck << ")=" << offset << ". Continue");
+            // XXX Likely that this will never be executed because if the upper


This introduces only extra logs and variable shortcuts, plus conditional ability to disable key packet sending, for development-testing purposes.

ethouris · 2020-01-20T18:23:24Z

srtcore/core.cpp

@@ -8017,6 +8185,9 @@ void CUDT::sendLossReport(const std::vector<std::pair<int32_t, int32_t> > &loss_

 int CUDT::processData(CUnit *in_unit)
 {
+    if (m_bClosing)
+        return -1;
+


The group code has given it more of a chance to receive a packet and process it while closing has started.

ethouris · 2020-01-20T18:29:29Z

srtcore/logging.h

@@ -51,7 +51,12 @@ written by
 // LOGC uses an iostream-like syntax, using the special 'log' symbol.
 // This symbol isn't visible outside the log macro parameters.
 // Usage: LOGC(mglog.Debug, log << param1 << param2 << param3);
-#define LOGC(logdes, args) if (logdes.CheckEnabled()) { srt_logging::LogDispatcher::Proxy log(logdes); log.setloc(__FILE__, __LINE__, __FUNCTION__); args; }
+#define LOGC(logdes, args) if (logdes.CheckEnabled()) \


This change improves the compiler error report for a case when someone forgot to pass the log marker in the call to LOGC and HLOGC macros. This enforces the interpretation of args as convertible to the type of log so that when someone mistakenly writes LOGC(mglog.Debug, "ACK received") the compiler will complain about inability to convert the string constant into the type of log.

ethouris · 2020-01-20T18:35:11Z

srtcore/platform_sys.h

@@ -93,6 +93,15 @@
 #include <unistd.h>
 #include <fcntl.h>

+#ifdef __cplusplus


This fixes some problems that appeared with the C API.

ethouris · 2020-01-20T18:35:59Z

srtcore/queue.cpp

+            HLOGC(mglog.Debug, log << "RID: socket @" << i->m_iID
+                << " removed - EXPIRED ("
+                // The "enforced on FAILURE" is below when processAsyncConnectRequest failed.
+                << (is_zero(i->m_tsTTL) ? "enforced on FAILURE" : "passed TTL")


Fix: when i->m_tsTTL is zero, it means that it was enforced on failure.

ethouris · 2020-01-20T18:37:28Z

srtcore/srt.h

 #define SRT_ATR_DEPRECATED
+#define SRT_ATR_DEPRECATED_PX
+
+// Nodiscard: issue a warning if the return value was discarded.


This is introduced because some compilers use prefix-type modifier, others use postfix-type modifier. This provides the attribute support for Microsoft compilers.

ethouris · 2020-01-20T18:38:11Z

srtcore/utilities.h

+inline RefType* AddressOf(RefType& r)
+{
+    return (RefType*)(&(unsigned char&)(r));
+}


This will be later used in some extra utilities.

ethouris · 2020-01-20T18:38:51Z

srtcore/utilities.h

+private:
+    template <class X>
+    explicit_t(const X& another);
+};


This is in order to prevent misuse of a changed API, while C++ allows a string constant to be converted to bool without making the developer wiser.

ethouris · 2020-01-24T16:16:58Z

Closed due to merged all dependent PRs.

Mikołaj Małecki added 30 commits December 9, 2019 19:43

Added construction from sockaddr. Added family override. Fixed addres…

db0f9c0

…s comparison for Mac

Added extra utilities

f081299

Added more utilities from final branch

c0a2107

Withdrawn changes for ref_t (to be removed)

ba70504

Removed callback

3d4d8e2

Application improvements

d443eef

Blocked an error-reporting type on Windows

29f8c0c

Fixed problems on Windows and Cygwin

6542a51

Added description for property macros

1eb4a4a

Prevent overload for size_t as it doubles the unsigned integer type

703b4c4

Added lacking include for shared_ptr

66d5907

Applied changes for family-independent socket

2dc0118

Merge branch 'dev-netinet-any-enhancements' into dev-unified-sockaddr

24e1886

Restored UDPSOCKET type to handle sockets portable way. Simplified ch…

cbf5530

…eck in SockaddrToString

Experimental: provide OptionScheme::operator= for VC 2013

4c85f14

Fixed windows part for the new sockaddr

2334b49

Experimental: removed version for =&&

d1c832d

Still lacking fixes for Windows

f255055

Fix to please Visual Studio 2013

de598dd

Fixed testapp build break

bfaaa10

Some further fixes after review

e6f57ff

Fixed wrong Ref use

366de56

Fixed wrong call on Windows

f3da784

Fixed initialization in LogConfig

5122e61

Fixed correct default argument for log_stream

e5ec784

Applied changes from sockaddr_any improvements

50d1b56

Merge branch 'master' into dev-unified-sockaddr

81762b8

Post-review fixes

488934b

Merge branch 'dev-lint-fixes-a1' into dr-pre-group-master

7e82f02

Merge branch 'dev-unified-sockaddr' into dr-pre-group-master

ad8868b

ethouris commented Jan 20, 2020

View reviewed changes

Updated from upstream (cleared already applied changes)

f1f2d68

ethouris closed this Jan 24, 2020

mbakholdina modified the milestones: v1.5.0, v1.4.2 Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring and debug improvements series from group code #1028

Refactoring and debug improvements series from group code #1028

ethouris commented Dec 19, 2019 •

edited

Loading

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris Jan 20, 2020

ethouris commented Jan 24, 2020

		@@ -110,13 +98,63 @@ CUDTSocket::~CUDTSocket()
		pthread_cond_destroy(&m_AcceptCond);
		}

		@@ -524,25 +681,12 @@ int CUDTUnited::installAcceptHook(const SRTSOCKET lsn, srt_listen_callback_fn* h
		return 0;
		}

		// a +% b : shift a by b
		// a == b : equality is same as for just numbers

Refactoring and debug improvements series from group code #1028

Refactoring and debug improvements series from group code #1028

Conversation

ethouris commented Dec 19, 2019 • 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethouris commented Jan 24, 2020

ethouris commented Dec 19, 2019 •

edited

Loading