> 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
