[asterisk-dev] [Code Review] 4542: DNS: Add NAPTR support and tests

[Mark Michelson]

> On March 31, 2015, 4:50 p.m., Kevin Harwell wrote:
> > /team/group/dns/main/dns_naptr.c, line 244
> > <https://reviewboard.asterisk.org/r/4542/diff/2/?file=73012#file73012line244>
> >
> >     This seems like it should be a non assert check. What happens if asterisk is not compiled without debug on and this is false?

The reason this is an assertion is because it should never be false.

Before regexp_repl_invalid is called, we divide the regexp based on the delimiter, and we ensure that we do not divide on backslash-escaped delimiters. This means that the regexp repl section cannot end with a backslash, unless it ends in a backslash-escaped backslash ("\\"). Current code would find the first backslash of the pair, and since the backslash is escaping an illegal character, the routine would return -1 before possibly finding the second backslash. If the code were altered to allow for backslash-escaped backslash, then the code would have to be altered from its current form and the assertion would need to be changed to a different type of check.


> On March 31, 2015, 4:50 p.m., Kevin Harwell wrote:
> > /team/group/dns/res/res_resolver_unbound.c, lines 1254-1291
> > <https://reviewboard.asterisk.org/r/4542/diff/2/?file=73013#file73013line1254>
> >
> >     Any reason to continue if a failure occurs?

Since previous failures don't affect future operations, my thought was to go through with the whole thing so that we can see if everything is failing or only a specific case. Of course the status update messages here aren't very helpful for determining what specific failure was seen, though. I can address that though.


> On March 31, 2015, 4:50 p.m., Kevin Harwell wrote:
> > /team/group/dns/tests/test_dns_naptr.c, lines 439-461
> > <https://reviewboard.asterisk.org/r/4542/diff/2/?file=73014#file73014line439>
> >
> >     Should these failures break the loop and just goto cleanup as well?

See my previous comment. A failure of one record doesn't necessarily mean the failure of another. This way, all failures can be on display if the unit test fails for whatever reason.


- Mark


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


On March 27, 2015, 2:45 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4542/
> -----------------------------------------------------------
> 
> (Updated March 27, 2015, 2:45 p.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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150331/2a3587f0/attachment-0001.html>