----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4556/#review15024 -----------------------------------------------------------
In general, I'm going to say that while I understand that this code was adapted from old code in Asterisk, this is is dire need of comments, both in the form of doxygen and comments over individual sections in functions like resolver_parse_answer(). This could use some off-nominal tests, specifically to ensure that malformed DNS responses do not cause crashes or other unexpected behaviors. /trunk/res/res_resolver_system.c <https://reviewboard.asterisk.org/r/4556/#comment25695> Curly braces on if statements /trunk/res/res_resolver_system.c <https://reviewboard.asterisk.org/r/4556/#comment25696> This has the same alignment concerns that I brought up in the SRV review. /trunk/res/res_resolver_system.c <https://reviewboard.asterisk.org/r/4556/#comment25697> Unless there's something I've missed, this appears to be passing the TTL in network byte order instead of host byte order. Add a check of the TTL in the nominal unit tests to be sure about this. /trunk/res/res_resolver_system.c <https://reviewboard.asterisk.org/r/4556/#comment25698> This isn't used. (yes, this is present in the NAPTR and SRV tests as well. I just didn't catch it until now) /trunk/res/res_resolver_system.c <https://reviewboard.asterisk.org/r/4556/#comment25699> The v4_buf isn't really needed here. You could just do: inet_pton(AF_INET, record->ip, ptr); ptr += V4_BUFSIZE; /trunk/res/res_resolver_system.c <https://reviewboard.asterisk.org/r/4556/#comment25693> There is a red blob at the end of this line. /trunk/res/res_resolver_system.c <https://reviewboard.asterisk.org/r/4556/#comment25692> Based on recent anecdotal experience, I think it's a good idea to impose a low maximum threadpool size for new threadpools we introduce. Something like 10 is probably a good default maximum (though even lower than that is probably fine). - Mark Michelson On March 29, 2015, 6:05 p.m., Joshua Colp wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4556/ > ----------------------------------------------------------- > > (Updated March 29, 2015, 6:05 p.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > ------- > > This change adds a res_resolver_system module which is a fallback for when > libunbound is not available. It's a port of the dns.c code into the new core > DNS API. While queries can be executed in an async fashion you can't cancel a > query that is in progress. There are also unit tests which cover the parsing > aspect of the code to make sure it works as expected. > > > Diffs > ----- > > /trunk/res/res_resolver_system.c PRE-CREATION > /trunk/configs/samples/resolver_system.conf.sample PRE-CREATION > > Diff: https://reviewboard.asterisk.org/r/4556/diff/ > > > Testing > ------- > > Ran unit tests, they are happy. > > > Thanks, > > Joshua Colp > >
-- _____________________________________________________________________ -- 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
