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.

Sign up for GitHub

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

Jump to bottom

Gateway RTP/SRTP bit pattern testing support #417

Closed
wants to merge 1 commit into from
Closed

Gateway RTP/SRTP bit pattern testing support #417

wants to merge 1 commit into from

Conversation

wdoekes
Copy link
Member

@wdoekes wdoekes commented Sep 16, 2019

(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.)

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Oct 2, 2019 via email

@wdoekes wdoekes added this to the 3.7 milestone Oct 30, 2019
@rkday
Copy link
Contributor

rkday commented Nov 3, 2019

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.

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Nov 4, 2019 via email

@rkday
Copy link
Contributor

rkday commented Nov 4, 2019

Thanks - get well soon!

@rkday rkday mentioned this pull request Nov 10, 2019
Closed
@wdoekes wdoekes force-pushed the feat/rtp-srtp-bit-testing-381 branch from a4b63a0 to 5fd4e51 Compare November 11, 2019 09:16
@jeannotlanglois @wdoekes
Gateway RTP/SRTP bit pattern testing support (SIPP-RTPCHECK-3.6)
f8b1064 
By @jeannotlanglois. Fixes by @orgads and @wdoekes.

[Detailed feature documentation available upon request]

NOTE: --with-rtpstream requires openssl here.
NOTE: Docs need updates too.
@wdoekes wdoekes force-pushed the feat/rtp-srtp-bit-testing-381 branch from ddf6dca to f8b1064 Compare November 11, 2019 10:09
@wdoekes
Copy link
Member Author

wdoekes commented Nov 11, 2019

Rebased and squashed all commits that could not be merged to master directly. (Parts of the squashed commit may possibly be cherry-picked sooner though.)

@wdoekes
Copy link
Member Author

wdoekes commented Nov 11, 2019

For instance:

  • DEFAULT_BEHAVIOR_BADCSEQ
  • EXIT_RTPCHECK_FAILED + pattern_id/loop_count + pauseapattern/etc..
  • prepare_pcap.c changes
  • Possibly all additional video/sdp/rtp code, but without the SRTP stuff

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Jan 27, 2020 via email

@wdoekes
Copy link
Member Author

wdoekes commented Jan 28, 2020

Hi! Glad to hear that you're on your feet again.

I believe the new sequence would be:

$ cmake . -DUSE_SSL=1
$ make -j12
$ ./sipp -v

 SIPp v3.6.0-4-gcc24f9d-TLS-RTPSTREAM.

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Jan 29, 2020 via email

@wdoekes
Copy link
Member Author

wdoekes commented Jan 30, 2020 •
edited
Loading

Could there be a minimum compiler version needed? I'm using gcc 4.4.7 on CentOS 6.9:

You are using heavily outdated stuff, including a machine with python2.6.

But the errors I see point to stray garbage in your directories instead.

/home/langloij/sipp-git/include/config.h:10:2: error: invalid preprocessing directive #cmakedefine

Try a:

$ git clean -nxd
Would remove CMakeCache.txt
Would remove CMakeFiles/
...

and then a:

$ git clean -fxd
Removing CMakeCache.txt
Removing CMakeFiles/
...

(After asserting that it's not removing anything important.)

That should get rid of that old config.h that's not supposed to be in include/:

$ cmake . -DUSE_SSL=1
$ find . -name config.h
./config.h

(Unless it's your cmake version that does something interesting with the paths..)

/home/langloij/sipp-git/include/stat.hpp:44:25: error: gsl/gsl_rng.h: No such file or directory
/home/langloij/sipp-git/include/stat.hpp:650: error: ISO C++ forbids declaration of ‘gsl_rng’ with no type

I think the new cmake stuff assumes gsl exists.. ( @rkday ).

Try -DUSE_GSL=:

$ cmake . -DUSE_GSL= -DUSE_SSL=1

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Jan 30, 2020 via email

@orgads
Copy link
Contributor

orgads commented Jan 30, 2020

Install libgsl-dev

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Jan 30, 2020 via email

@orgads
Copy link
Contributor

orgads commented Jan 30, 2020

This looks like a real error in the code. Follow the compiler error...

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Jan 30, 2020 via email

@orgads
Copy link
Contributor

orgads commented Jan 30, 2020

C++11 I suppose? What compiler do you use?

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Jan 30, 2020 via email

@orgads
Copy link
Contributor

orgads commented Jan 30, 2020

Use devtoolset-7

@wdoekes
Copy link
Member Author

wdoekes commented Jan 30, 2020

/home/langloij/sipp-git/include/stat.hpp:44:25: error: gsl/gsl_rng.h: No such file or directory

I did mention passing -DUSE_GSL= at the bottom of my reply. (@orgads: IMHO it should build fine if you don't have any gsl-devel and don't explicitly set USE_GSL=1.)

Is the current latest sipp code building successfully? or is it broken?

You can find succesful builds behind the green check mark at every commit:
https://github.com/SIPp/sipp/commits/master ->
https://github.com/SIPp/sipp/runs/412872308 ->
https://pipelines.actions.githubusercontent.com/qUVaVycWWqC7WFXb2RA1bmJzlebfHWHDxF3fd4yxdHrVNCVpNa/_apis/pipelines/1/runs/147/signedlogcontent/6?urlExpires=2020-01-30T17%3A01%3A42.8372809Z&urlSigningMethod=HMACV1&urlSignature=vPYEV%2BSRSncRTKVQjy0PGhuEc1WgMWJFLoXKLxVuIuU%3D
^-- 2020-01-28T13:02:51.7705208Z -- The CXX compiler identification is GNU 7.4.0 and a succesful build

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Feb 7, 2020 via email

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Feb 11, 2020 via email

@wdoekes
Copy link
Member Author

wdoekes commented Feb 12, 2020

There is a undefined reference to a "cbreak()" function which I can't see anywhere in SIPP source code:

Yes, screen.cpp uses it. It's declared somewhere in the libncurses-dev include files. And it's defined either in libncurses.so or libtinfo.so.

See this:

conda-forge/ncurses-feedstock#44

It looks like CMake isn't correctly using pkg-config to determine that -ltinfo should be added next to -lncurses.

Try this:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 50dfde9..1636c8b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -113,7 +113,16 @@ if(BUILD_STATIC)
   set(CMAKE_EXE_LINKER_FLAGS "-static-libgcc -static-libstdc++ -static")
 endif(BUILD_STATIC)
 
-find_library(CURSES_LIBRARY NAMES ncursesw cursesw ncurses curses)
+find_package(PkgConfig QUIET)  # import pkg_check_modules() and friends
+if(PKG_CONFIG_FOUND)
+  pkg_search_module(CURSES_LIBRARY ncursesw cursesw ncurses curses)
+  if(CURSES_LIBRARY_FOUND)
+    set(CURSES_LIBRARY ${CURSES_LIBRARY_LIBRARIES})
+  endif()
+endif()
+if(NOT CURSES_LIBRARY)
+  find_library(CURSES_LIBRARY NAMES ncursesw cursesw ncurses curses)
+endif()
 if(CURSES_LIBRARY)
   target_link_libraries(sipp dl ${CURSES_LIBRARY} pthread)
   target_link_libraries(sipp_unittest dl ${CURSES_LIBRARY} pthread gtest gmock)

And:

$ rm -rf CMakeCache.txt CMakeFiles/ && cmake . -DUSE_SSL=1 && make VERBOSE=1 -j4

This is a repeat of 90f38e4 / #271 but now for cmake.

The pkg-config stuff should probably be improved some, but my CMake foo is not up to speed...

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Aug 7, 2020 via email

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Aug 7, 2020 via email

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Aug 8, 2020 via email

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Aug 10, 2020 via email

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Aug 12, 2020 via email

@wdoekes
Copy link
Member Author

wdoekes commented Aug 17, 2020

What is SIPP's official position on the different OpenSSL versions? Should only the newer ones be supported or should there be backwards-compatibility for older ones as well?

Well. If it's not too much trouble, then an ifdef would be nice. Currently we're using:

#if OPENSSL_VERSION_NUMBER < 0x10100000 /*  MNNFFPPS: major minor fix patch status */

to check for < 1.1.

All of this is tucked away in sslsocket.cpp. Feel free to add more of those in newly added source files, but refrain from adding openssl-checks into existing files (other than sslsocket.cpp). We don't want openssl-implementation stuff leaking back into the rest of SIPp.

If it is too much trouble to add support for both openssl 1.1+ and 1.0, then I'd rather see the whole JLSRTP bit depend on openssl 1.1, so existing users are not affected by a dependency on newer openssl:

#if defined(USE_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x10100000
# define USE_JLSRTP
#endif

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Aug 17, 2020 via email

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Aug 20, 2020 via email

@jeannotlanglois
Copy link
Contributor

jeannotlanglois commented Aug 21, 2020 via email

@jeannotlanglois jeannotlanglois mentioned this pull request Aug 31, 2020
Merged
@wdoekes wdoekes closed this Sep 18, 2020
@wdoekes wdoekes deleted the feat/rtp-srtp-bit-testing-381 branch September 18, 2020 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs-testing
Projects
None yet
Milestone
3.7
Development

Successfully merging this pull request may close these issues.
4 participants
@wdoekes @jeannotlanglois @rkday @orgads