DNS: Add NAPTR support and tests
Review Request #4542 - Created March 27, 2015 and submitted
Mark Michelson | |
/team/group/dns/ | |
Reviewers | |
---|---|
asterisk-dev | |
Asterisk |
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.
All previous DNS tests continue to pass, and all new tests added in this review pass as well.
Review request changed
Updated (March 27, 2015, 9:45 a.m.)
-
- added Diff r2 - Show changes
Uncomment a test case from the nominal NAPTR test in test_dns_naptr.c
Posted (March 31, 2015, 10:25 a.m.)
-
/team/group/dns/include/asterisk/dns_internal.h (Diff revision 2) -
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 (Diff revision 2) -
Doxygen comment
-
/team/group/dns/main/dns_naptr.c (Diff revision 2) -
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 (Diff revision 2) -
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 (Diff revision 2) -
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.
Posted (March 31, 2015, 11:50 a.m.)
-
/team/group/dns/include/asterisk/dns_internal.h (Diff revision 2) -
These methods should not start with ast_* unless they are meant to be exposed externally/exported. If they are then they should be moved to a more "public" include file.
-
/team/group/dns/main/dns_naptr.c (Diff revision 2) -
This seems like it should be a non assert check. What happens if asterisk is not compiled without debug on and this is false?
-
/team/group/dns/res/res_resolver_unbound.c (Diff revision 2) -
Any reason to continue if a failure occurs?
-
/team/group/dns/tests/test_dns_naptr.c (Diff revision 2) -
Should these failures break the loop and just goto cleanup as well?
Review request changed
Updated (April 1, 2015, 9:51 a.m.)
-
- added Diff r3 - Show changes
Addressed review feedback from Matt and Kevin. * Added Doxygen comments for NAPTR data and DNS record data_ptr * Added safety checks to return NULL if NAPTR record not found in DNS result. * Macrotized the check if we are past the end of the NAPTR record when parsing. * Short-circuit the sort operation if fewer than two NAPTR records are returned * Removed "ast_" prefix from internal DNS routines. * Error messages for nominal tests are more descriptive
Ship It!
Review request changed
Updated (April 7, 2015, 6:11 a.m.)
- changed from pending to submitted
Committed in revision 434211
Other reviews