Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gateway RTP/SRTP bit pattern testing support (SIPP-RTPCHECK-3.6) #481

Merged
merged 1 commit into from
Sep 16, 2020

Conversation

jeannotlanglois
Copy link
Contributor

[Detailed feature documentation available upon request]

Hello @wdoekes, @rkday, @orgads:

(I'm still relatively new to GitHub and I'm doing everything from the commandline through SSH so let me know if there are things that I'mnot doing quite right. But I figure that "Pull Request" is how one does code view request prior to a merge.)

Nearly a year ago in September 2019 I had submitted a pull request with my gateway RTP/SRTP bit pattern testing support (codename "RTPCHECK"). I finally got back in good health and found some time to work on fixing most (but not all yet) items that were pointed out. I've been allowed to work on this for 4 weeks straight now but I have to resume normal work so it'll take a little while before I can continue working on this.

As this is quite a big change, I'd prefer (at your discretion of course) if this functionality could be merged sooner than later as it is becoming quite complex to rebase/reintegrate every 6 months. Now at least I've finally started actively using the SIPP 3.6 branch in all my work so I no longer need to put effort into backporting into SIPP 3.5 for my own private use. But still this remains a big change.

So below I'm trying to summarize all the items that you pointed out in 2019 and indicating which ones were addressed and which ones have not (and why). So at your convenience please take time to review and determine if things are "good enough" for merge or if more work is necessary (which I'll try to do once more time becomes available).

I believe my feature would be for the SIPP 3.7 branch, so a new branch would likely need to be created before merging. This is probably not for me to do. Please advise.

.
.
.

From: Orgad Shaneh [email protected]
Sent: Tuesday, September 03, 2019 6:01 AM
To: SIPp/sipp [email protected]
Cc: Jeannot Langlois [email protected]; Author [email protected]
Subject: Re: [SIPp/sipp] Gateway RTP/SRTP bit pattern testing support (SIPP-RTPCHECK-3.6) (#381)

Hi,
I rebased it for you. Checkout https://github.com/orgads/sipp/commits/srtp

(Jeannot) I learned the GitHub workflow some more now and I did create my own repository at https://github.com/jeannotlanglois/sipp -- as of August 25th 2020 the master branch is still up to date and the feature code is in the sipp-36x-rtpcheck branch. So I can take care of the rebases myself now.

I also fixed build on Windows, and against openssl 1.1.

(Jeannot) I did a lot of testing with Linux (CentOS 6.9 using devtoolset-7) as well as fixing all the integration issues (CI testcases) for Darwin. I do have a CygWin testing environment but however did not have enough time to try it yet. If you want feel free to give a try at a CygWin build of my sipp-36x-rtpcheck git branch.

(Jeannot): As for openssl I've moved the JLSRTP component to use the proper EVP_xxx() APIs so there should be no issues with that library nymore. Also note that I've put in the extra effort to make SRTP-related code optional as Rob Day had mentioned in case some folks are not allowed to compile with -DUSE_SSL=1 and must use -DUSE_SSL=0 instead (e.g. only RTP, no SRTP).

The changelog entry that you removed looks irrelevant, as 3.6 was already released, so this is for 3.7. You may add a new entry >for 3.7.

(Jeannot) I think I had reverted that 3.6 part. But whoops, I did not do anything (yet) for 3.7. Please indicate any details you'd like me to indicate there.

There are many warnings on Windows regarding conversion of pthread_t to int (mostly on printf's). I worked around those who >are considered errors (when passing pthread_t to a function that expects int), by casting to long long and then to int, but you >should consider really fixing it..

(Jeannot) I'm fairly sure that those warnings have been addressed (when I started using devtoolset-7 with gcc 7.3.1 I had to fix a bunch of these for printfs -- so in all likelihood the windows build should no longer trigger those.

.
.
.

-----Original Message-----
From: Walter Doekes [email protected]
Sent: Monday, September 23, 2019 10:23 AM
To: Jeannot Langlois [email protected]
Subject: Re: [SIPp/sipp] Gateway RTP/SRTP bit pattern testing support (#417)

Hi Jeannot,

I'm sorry to hear that. I hope your recovery goes a swiftly as it possibly can.

Don't worry about the followup. I'll get to it when I get to it. I can't promise anything either ;)

If you have a quick example of what commands you were running when using your new codec/rtp functionality, a quick sample >would be appreciated.
But not mandary.

(Jeannot) Yeps -- even before you mentioned this I had already addressed this concern by having two sample text files in the main SIPP working directory list sample command lines ("SET-SIDE" means "UAC" and "ICP-SIDE" means "UAS", I should probably change that though):

For RTP bit pattern testing:
`
SET-SIDE command line:
./sipp 192.168.1.250:5060 -sf sipp_scenarios/pfca_uac_pattern.xml -i 192.168.1.147 -t u1 -p 5060 -mp 4000 -m 1 -s 16002

ICP-SIDE command line:
./sipp -sf sipp_scenarios/pfca_uas.xml -i 192.168.1.250 -t u1 -p 5060 -mp 5000 -m 1 -s 16001 -rtp_echo
`

For SRTP bit pattern testing:
`
SET-SIDE command line:
./sipp 192.168.1.250:5060 -sf sipp_scenarios/pfca_uac_bpattern_crypto_simple.xml -i 192.168.1.147 -t u1 -p 5060 -mp 4000 -m 1 -s 16002 -rtpcheck_debug -srtpcheck_debug

ICP-SIDE command line:
./sipp -sf sipp_scenarios/pfca_uas_both_crypto_simple.xml -i 192.168.1.250 -t u1 -p 5060 -mp 5000 -m 1 -s 16001 -srtpcheck_debug
`

.
.
.

From: Jeannot Langlois
Sent: Wednesday, October 02, 2019 12:42 PM
To: SIPp/sipp reply+AJK3K5LIN7YGSC2XRVL7W3V3RTWN5EVBNHHB246J4A@reply.github.com; SIPp/sipp >[email protected]
Cc: Mention [email protected]
Subject: RE: [SIPp/sipp] Gateway RTP/SRTP bit pattern testing support (#417)

Hi there,

Trying to follow-up on this email:

"
(By @jeannotlanglois and rebased by @orgads, as found in #381.)
I fixed some code/tests so Travis and the baseline tests are happy again.
I'm not sure the documentation is complete:
• For instance, why there are that many scenario files?
• Is the rtp_stream= keyword change sufficiently documented?
• Documentation that RTPSTREAM now depends on SSL (splitting that up again is a major pain). (And openssl-1.0.2+ is >required. Missing on Ubuntu/Trusty in Travis at least.)
• Checking/confirming that -mp + -max_rtp_port behave well together.
• Whether [media_port] or [rtpstream_audio_port] should be used (and when).
• How to retrieve RTP error status codes / logs.
Also, a bit of code linting would be nice. (Yay for spaces around operators..)
Also, check if the new cmdline arguments all have proper defaults / have a sane name.
(And splitting off 3.6 and creating 3.7 before merging this, is a good idea.)
"

  1. The many scenario files re provided as examples. I have attempted to send a detailed document explaining every single item >to walter doeke's email address in the past but it appears that my email was rejected. Let me know how I can provide the PDF >of the documentation to you. It includes numerous examples with regards to ALL XML syntax changes. It also includes >examples as to how [media_port] and [rtpstream_audio_port] need to be used.

(Jeannot) This explanation is still valid. I have the full documentation available in a PDF document (exported from an internal Confluence page) with a few illustrations showing the signalling/streaming flow directions, as well as examples of every possible syntax. Maybe an ASCII version might need to be drafted however if PDF is not adequate.

  1. I did not mention explicitly that RTPSTREAM depends on SSL however.

(Jeannot) This concern was also brought by Rob Day and I have addressed it -- I have went through the extra effort of splitting RTP + SRTP so that the latter can be built optionally from the former depending on whether -DUSE_SSL=1 or -DUSE_SSL=0 is specified which should take care of concerns about those who are not allowed to use OpenSSL.

  1. I don't believe that I've tested "-mp" and "-max_rtp_port" interactions.

(Jeannot) This concern remains valid -- I have not performed any such tests yet. Maybe some extra details would help here as to what could be affected (if anything).

  1. The RTP error status codes can be obtained like any other error codes from the command lines (by example under linux, in >bash, using the "$?" environment variable right after sipp returns). The RTP/SRTP logs are dumped in the sipp execution >directory.

(Jeannot) This explanation remains valid.

Taking for granted that SIPP-RTPCHECK (e.g. this feature) would be part of sipp-3.7.

(Jeannot) This is also still valid.

Taking notes of the above for my own reference when I can get back to this side project (hopefully later in 2019).

(Jeannot) Turns out it took a year but I finally got back to this :)

.
.
.

From: Rob Day [email protected]
Sent: Sunday, November 03, 2019 6:57 PM
To: SIPp/sipp [email protected]
Cc: Jeannot Langlois [email protected]; Mention [email protected]
Subject: Re: [SIPp/sipp] Gateway RTP/SRTP bit pattern testing support (#417)

I'm a bit cautious about making rtpstream depend on OpenSSL - per #32, distributing SIPp with OpenSSL support is a bit dicey, >and this would mean that distributions like https://packages.debian.org/sid/comm/sip-tester ("built...without openssl due licenses >incompatibilities") could no longer support rtpstream.

(Jeannot) As I mentioned above this concerned has now been addressed -- RTPSTREAM can still be built without OpenSSL support and yet still use just the RTP part of RTPCHECK. The SRTP part of RTPCHECK can now be built only when -DUSE_SSL=1 is in use.

.
.
.

Alright -- that completes all the items I had on my list for now.

[Detailed feature documentation available upon request]
@wdoekes
Copy link
Member

wdoekes commented Sep 1, 2020

Thanks @jeannotlanglois for all the effort.

I branched master into branch/3.6 now so any bug fixes to 3.6.x can be done there.

All new features should be merged into master which will at one point become 3.7.0.

@rkday @vodik: I'm planning on merging all of this into master (3.7.x). I'm sure there are some things to argue over and fix, but that can be done after the merge. Please speak up if you disagree.

@wdoekes wdoekes added this to the 3.7 milestone Sep 1, 2020
@wdoekes wdoekes merged commit 8c30e3d into SIPp:master Sep 16, 2020
uhle added a commit to uhle/sipp that referenced this pull request Feb 8, 2021
(pulled in by SIPp#481)

- missing parentheses within if condition
- comparison between signed and unsigned integer values
- dereferencing an uninitialized pointer
- variable-length array being unsupported by the C++ standard
uhle added a commit to uhle/sipp that referenced this pull request Feb 8, 2021
(pulled in by SIPp#481)

It was fixed years ago by commit 0026639 (PR SIPp#129).
wdoekes pushed a commit that referenced this pull request Oct 25, 2021
(pulled in by #481)

- missing parentheses within if condition
- comparison between signed and unsigned integer values
- dereferencing an uninitialized pointer
- variable-length array being unsupported by the C++ standard
wdoekes pushed a commit that referenced this pull request Oct 25, 2021
(pulled in by #481)

It was fixed years ago by commit 0026639 (PR #129).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants