Merge with Sangoma's sipp fork

[vodik]

Hey, so we have our own internal sipp fork, started off of sipp-3.2 release, which we would like to spend some time merging upstream. We currently don't have it hosted online here yet... but there's quite a bit of work you guys might be interested in. Some stuff I listed may or may not have been done again independantly - I haven't had a chance to fully discover this repo's code base, so sorry if I list what appears a non-feature

• Decoupled sipp from ncurses so sipp behaves correctly when stdout isn't a tty (I want to get to the point where its fully optional)
• Reworked pcap playback: support for more pcap datalinks (only DTL_LINUX_SLL tested atm) and setting a pcap search directory
• Ability to inject additional headers into a sipp script from command line arguments (we wanted something a little more powerful then variable expansion)
• Slightly more logging
• Misc fixes in ipv6 handling (things that I think have been fixed already) and source port/address binding for tcp

[wdoekes]

Well, all patches are welcome of course. Thanks!

I personally like the idea of using it without ncurses. As for the others:

• some work has been done on the pcap front, but it's stale, so that will probably have to be redone anyway (see No need to be root for pcapplay. #57)
• several existing log messages have been touched (in 3d8acc2 -- not in master yet)
• there have indeed been some ipv6 fixes, iirc

[vodik]

Alright, so my time has opened up a bit and I'm going to try and get the rest of this pushed through. Hopefully the next set of stuff should be ncurses decoupling. It just a bit painful to backport changes since I'm porting pre-uncrustified diffs 😭.

Is there any interest in a python/pytest wrapper around sipp? We got one kicking around, I don't know if you guys would be interested in it.

[wdoekes]

support for more pcap datalinks (only DTL_LINUX_SLL tested atm)

Has been added just now in bc7b536.

Is there any interest in a python/pytest wrapper around sipp? We got one kicking around, I don't know if you guys would be interested in it.

Not sure. I already made a few tests with bash in the /regress/ directory. I don't have an opinion about moving to python, but it shouldn't hurt.

[vodik]

Well its not tests for sipp, its for using sipp to test something else in an automated way. Scenario testing.

[wdoekes]

Then I'm not sure it fits in here. You can always throw it on github and see it interest is sparked.

I bet there are a lot of python (and ruby, and bash) test suites that incorporate calls to one or several sipp instances; all with their own advantages and disadvantages.

[vodik]

Fair enough

[vodik]

Okay, so here's the status:

• Most of the bugs I found have been already fixed here. Namely handling ipv6 addresses segfaulting due to poor parsing and tcp transport not binding to the user_port under some conditions. SDP parsing was the only issue I found still broken (and its been fixed).
• Our curses stuff has been merged. This was necessary to run sipp in an automation framework (pytest).
• The quote handling has been landed which fixed sipp breaking on lxml generated sipp scenarios.
• DTL_LINUX_SLL has been merged (thanks @wdoekes, saves me the work).

There is only one other thing I want to get upstream which I think is useful, but needs some care to do nicely -- we did it as a quick hack:

• We extended -trace_screen by adding an additional flag: -screen_file. If set, we override the placement of log files for cleaner reporting/logging code (once again, remember we have an automation framework built around sipp). I don't think the command line flag is the most descriptive and this could do with some refactoring.

Stuff were we we still diverge that might be of interest but needs some care (and I'm curious if there's interest):

• We changed the default logic in pcap_play so search the same folder the xml scenario is found in instead of the current working directory. This might be desirable since it eases sharing/organization. However outright changing the search logic might break existing systems that expect search in the current working directory. We could manually search the current working directory and then fail over to searching the scenario's path. This needs some addition work because it was also a quick hack and I wouldn't want to do this without a general helper so the same consistent search logic can be used elsewhere (like the new t38 support).
• We added logic to be able to inject headers from the sipp command line. Usage like this: -xfilter INVITE -xheader 'X-ExtendedTestHeader1: Hello World"' which injects that particular header on the first INVITE message. I was asked to write this but currently we've yet to use it.

Now that said, going forward, this isn't just a dump-and-run type merge. We'll be using this repository and maintaining our work here going forward. I've got explicit permission from the guys upstairs 😄.

Internally we've been considering replacing SIPp with something else because of a lot of the shortcoming with the scenario files (autogenerating them isn't that great of a workaround either) and with reporting. Instead we've decided to try and extend SIPp so it can be hooked into from a scripting language like Python. Hopefully you guys are open to this kind of work.

[vodik]

Okay, diving into the code looks like there's already a -screen_file which does something different from what we do with it exactly what we do. I guess a lot has happened since 3.2.

[wdoekes]

• pcap_play logic: I'm fine with that change, using CWD to find the pcap sounds like a bug, but we do need some kind of migratory arrangement. The simplest form would be to add a boolean to the exec element: <exec pcap_play_audio="..." relpath="true"/> or (less nice) use a new attribute <exec pcap_play_audio_relative="..."/>. Or a boolean option on the command line. Or something completely different.
• I'm not fond of the -xfilter INVITE syntax, but it is nice to be able to add headers. Can't it be done with regular -key custom_header "X-Header: stuff"? And if not, what would need to change?

[vodik]

The simplest form would be to add a boolean to the exec element:

I think that over complicates it a bit. Why not just try to load the path and only if it fails search next to the scenario data? That keeps backwards comparable behaviour. What are the odds someone is going to have pcaps and so forth in both places?

Can't it be done with regular -key custom_header "X-Header: stuff"? And if not, what would need to change?

Very little and there's really no reason it couldn't be done, considering how sipp parses command line flags (say compared to getopt_long).

[moises-silva]

I think that over complicates it a bit. Why not just try to load the path and only if it fails search next to the scenario data? That keeps backwards comparable behaviour. What are the odds someone is going to have pcaps and so forth in both places?

+1, in fact, that was supposed to be the logic we have in our current branch, isn't it? no need to break backwards compatibility

We added logic to be able to inject headers from the sipp command line. Usage like this: -xfilter INVITE -xheader 'X-ExtendedTestHeader1: Hello World"' which injects that particular header on the first INVITE message. I was asked to write this but currently we've yet to use it.

I remember we did this, but can't remember the use case?. If it's not used by our regression I'd say it does not belong upstream yet (no point in adding a feature whose value is still iffy) and we should just keep the code around in our branch in case we truly need it later

[vodik]

@moises-silva yeah, agreed. I can't remember why we did it either. But its worth bringing up so if someone asks, at least they know its been done.

[wdoekes]

As for loading from both places, that does sound less complicated indeed :P

I'd go as far as to say that it should first try from the location relative to the scenario, and first then from the CWD:

• you could certainly have a stray pcap file in your working junk dir (I leave lots of junk in the root of my working dir), you'll almost certainly want the one beside the xml.
• if we add a warning (stderr) that we take the scenario-relative file and not the cwd-relative file (if both are found), that should be sufficient notification.

[vodik]

Sounds good. We did a quick hack, so I'm going to spend a bit more time on this.