Use heuristic SIP parser by default.#2163
Conversation
The two parseSipLayer methods are functionally identical, except that the removed requires ports. Port numbers by themselves should not invalidate a parse and should only be used as hints to determine which parser to try first.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2163 +/- ##
=======================================
Coverage 82.72% 82.72%
=======================================
Files 332 332
Lines 59807 59785 -22
Branches 12593 12579 -14
=======================================
- Hits 49475 49457 -18
+ Misses 8944 8939 -5
- Partials 1388 1389 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@Dimi1010 the port-based parser does less work compared to the heuristic parser: it checks the port first - if it seems like a request it only tries to parse the method and then determines it's a SIP request. If it seems like a response it only tries to parse the version and then determines it's a SIP response. The heuristic parser parses the first 3 chars, but then if it seems like a request or a response it parses the entire first line which is more than only checking the method / version. We had a long discussion in #2024 and decided to build these 2 parsers because in most cases SIP is identified by its port and we'd like to parse it faster in this case. |
|
@seladb I see. On closer look, I think version that parses the first line has a higher ceiling of optimization, tho.
|
|
@Dimi1010 you're right that the first line is parse in the SIP layer constructors, but now with the 2 parsers the first line is actually parsed twice (one in the parser and then again in the constructor). I think that a better optimization could be lazy parsing of the first line, but I don't think it's going to be easy |
|
@seladb yeah, converted this to a draft for now because of that. I will try to implement the the reuse of the first line parse and then revisit. A lazy line parse could be better, but if the line is already needed to validate the parser, it might be of limited benefit. |
|
@Dimi1010 in the common case where the port is known, it might be better to lazy parse the first line. In the heuristic parser I agree it doesn't add anything because the first line parsing is needed to identify the layer |
2 participants
The PR deduplicates the
parseSipLayerlogic by removing the overload that takes a port src / dst pair.The existing two parser overloads perform effectively identical, with the exception that the port based overload also requires a specific port combination. Port numbers by themselves should generally only be considered hints for the next layer type and not a parsing error by themselves. As such, they shouldn't be part of downstream parsers that validate the data contents. Instead they should only be used during parser dispatch to select the most likely parser candidate.