----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4542/#review14972 -----------------------------------------------------------
/team/group/dns/include/asterisk/dns_internal.h <https://reviewboard.asterisk.org/r/4542/#comment25588> Doxygen comment. In particular, the comment should explain the relationship of data_ptr to data, and why it is necessary. /team/group/dns/include/asterisk/dns_internal.h <https://reviewboard.asterisk.org/r/4542/#comment25589> Doxygen comment /team/group/dns/main/dns_naptr.c <https://reviewboard.asterisk.org/r/4542/#comment25591> The asserts here are appropriate. However, if there is an error in the record, such that the memcmp never returns true, we'll get stuck in this loop. It may be good to have a 'fail safe' break out in the loop, after the asserts. That way in dev-mode we'll catch issues, but in production systems, the allocation of the NAPTR record will fail and we can (hopefully) gracefully handle it. /team/group/dns/main/dns_naptr.c <https://reviewboard.asterisk.org/r/4542/#comment25592> Suggestion: since this is repeated after each check, you may want to macro-tize it: #define CHECK_END_OF_RECORD do { \ if (ptr => end_of_record) { \ return NULL; \ } } while (0) Then you can just put: ptr += 2; CHECK_END_OF_RECORD; Or something like that. /team/group/dns/main/dns_naptr.c <https://reviewboard.asterisk.org/r/4542/#comment25590> I'd check to make sure num_records is non-zero before allocating the array. If it is zero, you can simply bail out of the routine. - Matt Jordan On March 27, 2015, 9:45 a.m., Mark Michelson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4542/ > ----------------------------------------------------------- > > (Updated March 27, 2015, 9:45 a.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > ------- > > This adds NAPTR support for DNS in Asterisk. > > The main parts of this are the functions for allocating a DNS NAPTR record > when a resolver wishes to add a NAPTR record, the sorting algorithm for > sorting DNS NAPTR records, and the tests that use a mock DNS resolver. > > NOTE: There is likely to be some overlap here in this review and Josh's SRV > review (/r/4528). Our stance on this is that we will factor out the > duplicated code once both SRV and NAPTR have been merged into the main DNS > branch. The factoring out of common functions will be placed in its own > review. > > > Diffs > ----- > > /team/group/dns/tests/test_dns_naptr.c PRE-CREATION > /team/group/dns/res/res_resolver_unbound.c 433573 > /team/group/dns/main/dns_naptr.c 433573 > /team/group/dns/main/dns_core.c 433573 > /team/group/dns/include/asterisk/dns_internal.h 433573 > > Diff: https://reviewboard.asterisk.org/r/4542/diff/ > > > Testing > ------- > > All previous DNS tests continue to pass, and all new tests added in this > review pass as well. > > > Thanks, > > Mark Michelson > >
-- _____________________________________________________________________ -- 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
