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

Tue Feb 11 10:06:14 CST 2014

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


/branches/12/include/asterisk/res_hep.h
<https://reviewboard.asterisk.org/r/3207/#comment20448>

    Two things:
    
    When having a single-bit bitfield, you probably should make this unsigned.
    
    Move this to the end of the structure.


/branches/12/include/asterisk/res_hep.h
<https://reviewboard.asterisk.org/r/3207/#comment20449>

    s/with be/will be/
    
    or
    
    s/with be/is/


/branches/12/include/asterisk/res_hep.h
<https://reviewboard.asterisk.org/r/3207/#comment20450>

    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.


/branches/12/res/res_hep.c
<https://reviewboard.asterisk.org/r/3207/#comment20451>

    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.


/branches/12/res/res_hep.c
<https://reviewboard.asterisk.org/r/3207/#comment20452>

    Place all the CHUNK_TYPE_* defines in an enum.


/branches/12/res/res_hep.c
<https://reviewboard.asterisk.org/r/3207/#comment20455>

    Document that all these hep structures store their content in network byte order.


/branches/12/res/res_hep.c
<https://reviewboard.asterisk.org/r/3207/#comment20456>

    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.


/branches/12/res/res_hep.c
<https://reviewboard.asterisk.org/r/3207/#comment20454>

    I believe you're leaking capture_info here. Given the multitude of return paths, RAII_VAR is probably a good fit for it here.


/branches/12/res/res_hep.c
<https://reviewboard.asterisk.org/r/3207/#comment20458>

    Looks like you left in some debugging.


/branches/12/res/res_hep_pjsip.c
<https://reviewboard.asterisk.org/r/3207/#comment20457>

    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)


- Mark Michelson


On Feb. 11, 2014, 12:21 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3207/
> -----------------------------------------------------------
> 
> (Updated Feb. 11, 2014, 12:21 p.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/20140211/bbe444f3/attachment-0001.html>