[asterisk-dev] [Code Review] 3207: HEP: Add a Homer Encapsulation Protocol (HEP) v3 capture agent module and a packet logger for PJSIP

[Matt Jordan]

> On Feb. 11, 2014, 10:06 a.m., Mark Michelson wrote:
> > /branches/12/include/asterisk/res_hep.h, lines 76-78
> > <https://reviewboard.asterisk.org/r/3207/diff/2/?file=53825#file53825line76>
> >
> >     I don't understand this line. Does this refer to the payload parameter to this function? If so, then reword this to be more explicit. If not, then...I have no idea what this means.

It's the payload and the uuid, really.

It's saying that the hepv3_capture_info object that is returned from this function has pointers to things. If you point those things to something on the stack, that's a bad idea, and you're going to have a bad time.

I'll clarify.


> On Feb. 11, 2014, 10:06 a.m., Mark Michelson wrote:
> > /branches/12/res/res_hep.c, lines 92-93
> > <https://reviewboard.asterisk.org/r/3207/diff/2/?file=53826#file53826line92>
> >
> >     This is a very strange default for the HEP server. I suspect the port is common, but the IP address is making an odd assumption.

I agree, it was in Alexandr's original code and I kept it. I'll set the value to "", and if we don't have an IP address, the way the option is registered will cause the module load to fail.


> On Feb. 11, 2014, 10:06 a.m., Mark Michelson wrote:
> > /branches/12/res/res_hep.c, lines 197-200
> > <https://reviewboard.asterisk.org/r/3207/diff/2/?file=53826#file53826line197>
> >
> >     I think there's an issue with this and the hep_chunk_payload structure. If this is expected to be sent over the wire, then you can't send a pointer. You need to have a buffer the size of the chunk's length field and copy the data into that buffer.
> >     
> >     EDIT: After looking further at the code, it appears that this and hep_chunk_payload aren't actually being used. The payload is being sent by manually placing a hep_chunk and then the payload onto the socket buffer. To discourage the use of hep_chunk_str and hep_chunk_payload, you should probably just get rid of these structures.

Yup, I'll just remove them.

Some of the other HEP data items (not HEPv3) are also leftovers; I'll remove them as well.


> On Feb. 11, 2014, 10:06 a.m., Mark Michelson wrote:
> > /branches/12/res/res_hep.c, lines 431-435
> > <https://reviewboard.asterisk.org/r/3207/diff/2/?file=53826#file53826line431>
> >
> >     I believe you're leaking capture_info here. Given the multitude of return paths, RAII_VAR is probably a good fit for it here.

Egads. Fixed.


> On Feb. 11, 2014, 10:06 a.m., Mark Michelson wrote:
> > /branches/12/res/res_hep_pjsip.c, lines 62-68
> > <https://reviewboard.asterisk.org/r/3207/diff/2/?file=53828#file53828line62>
> >
> >     Is this the intended use of the UUID in capture info? The same Call-ID will be repeated over many packets.
> >     
> >     (Same comment applies for the logging_on_rx_msg function)

Yes, that's the intended purpose. It's used to tie messages together, not necessarily to give each individual message a unique identifier.


- Matt


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3207/#review10851
-----------------------------------------------------------


On Feb. 11, 2014, 6:21 a.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3207/
> -----------------------------------------------------------
> 
> (Updated Feb. 11, 2014, 6:21 a.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp and Olle E Johansson.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch adds the following:
> (1) A new module, res_hep, which implements a generic packet capture agent for the Homer Encapsulation Protocol (HEP) version 3. Note that this code is heavily based on a patch provided by Alexandr Dubovikov; I basically just wrapped it up, added configuration via the configuration framework, and threw in a taskprocessor.
> (2) A new module, res_hep_pjsip, which performs packet capturing for the PJSIP SIP stack. This is one of those modules that I think really showcases how nice the new stack is - we're able to add a new module that inserts itself into the stack and forwards the message traffic off to the res_hep module without modifying the core parts of the stack itself. This means a system administrator could load this at will on certain Asterisk systems and - if the capturing isn't needed - unload it and keep the stack 'slim'.
> 
> A few notes:
> 
> * This code exists in the following branch:
>   http://svn.asterisk.org/svn/asterisk/team/mjordan/12-hep
> * The code in the branch also contains a module for RTCP. While that actually *does* send RTCP information over HEP, it does so as a JSON blob, which is not super useful. It's an open question as to what the formatting should be, i.e., a SNOM-esque encoding, RFC 6035, etc. I'm open to suggestions on this, which is why I deferred that functionality for a later review.
> * Much thanks to Alexandr for his Asterisk patch for this code and for a *lot* of patience waiting for me to port it to 12/trunk. Due to some dithering on my part, this has taken the better part of a year to port forward (I still blame CDRs for the delay).
> 
> 
> Diffs
> -----
> 
>   /branches/12/res/res_hep_pjsip.c PRE-CREATION 
>   /branches/12/res/res_hep.exports.in PRE-CREATION 
>   /branches/12/res/res_hep.c PRE-CREATION 
>   /branches/12/include/asterisk/res_hep.h PRE-CREATION 
>   /branches/12/configs/hep.conf.sample PRE-CREATION 
>   /branches/12/CHANGES 407945 
> 
> Diff: https://reviewboard.asterisk.org/r/3207/diff/
> 
> 
> Testing
> -------
> 
> An automated test that emulates a SIP capture server was written and is up for review here: https://reviewboard.asterisk.org/r/3206
> 
> This admittedly needs some *real* testing, as I have yet to stand up Kamailio with HEP. I think the code is far enough along to get some eyes on it however.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140214/d4aba874/attachment-0001.html>