[asterisk-dev] [Code Review] 4474: core: Add basic DNS API implementation

[Mark Michelson]

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


Since there is a dns_internal header, it seems prudent to put the different definitions in different source files as well. In other words, have dns_naptr.c, dns_recurring.c, etc.


/trunk/include/asterisk/dns_recurring.h
<https://reviewboard.asterisk.org/r/4474/#comment25195>

    This note should reference ast_dns_resolve_recurring_cancel instead of ast_dns_resolve_cancel



/trunk/main/dns_core.c
<https://reviewboard.asterisk.org/r/4474/#comment25201>

    Seeing this code (and the code in dns_query_recurring_get_ttl()) brings up an interesting discussion point. How should Asterisk be treating TTL of 0 in recurring queries?
    
    Let's examine two scenarios. In scenario A, Asterisk performs a recurring DNS lookup of a domain and receives a response with two records, one with TTL 0 and one with TTL of INT_MAX. In scenario B, Asterisk performs a recurring DNS lookup of a doman and receives a response with one record, whose TTL is 0.
    
    Currently, in scenario A, Asterisk will schedule the recurring query for INT_MAX seconds from now*, since the 0 TTL record is ignored. Currently, in scenario B, Asterisk will not schedule a recurring query at all.
    
    Obviously, you can't interpret a 0 TTL to mean that you should constantly spam the DNS server with queries. So what should you do instead? It may be that the core as written is correct. It is then the responsibility of the user of the recurring query API to examine the TTL of returned records and switch from using a recurring query to something else. Or it may be that the recurring query API can make this transition easier for users ... somehow.
    
    I think that no matter where this discussion goes, something needs to be documented somewhere about this.
    
    
    
    
    * Actually this would currently overflow. Probably should address that as well.



/trunk/main/dns_core.c
<https://reviewboard.asterisk.org/r/4474/#comment25197>

    The comment and code here don't match up. The comment says you're bumping the refcount of recurring, but you don't actually do it in the call to ast_dns_resolve_async().
    
    I think in this case, the comment is correct. As it stands, if a caller calls ast_dns_resolve_recurring() and then calls ao2_cleanup() on the returned value before the async resolution completes, then the async resolution callback will attempt to manipulate a freed object.



/trunk/main/dns_core.c
<https://reviewboard.asterisk.org/r/4474/#comment25198>

    If the call to ast_dns_resolve_cancel() returns non-zero, should ast_dns_resolve_recurring_cancel() return non-zero as well? On the one hand, you did unschedule the recurring query, but whatever one currently in flight did not get canceled.


- Mark Michelson


On March 11, 2015, 11:42 a.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4474/
> -----------------------------------------------------------
> 
> (Updated March 11, 2015, 11:42 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24834
>     https://issues.asterisk.org/jira/browse/ASTERISK-24834
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This change implements the basic API as described on the DNS API wiki page. Minimal changes have been made as required based on real usage and getting a feel for it. As it is the core functionality is present. Resolvers can register, queries can be made (async / sync).
> 
> As the API was basically copy/pasted from the wiki page there still remain stubs to be filled in for higher level functionality (such as parsing and query sets).
> 
> 
> Diffs
> -----
> 
>   /trunk/main/dns_query_set.c PRE-CREATION 
>   /trunk/main/dns_core.c PRE-CREATION 
>   /trunk/include/asterisk/dns_tlsa.h PRE-CREATION 
>   /trunk/include/asterisk/dns_srv.h PRE-CREATION 
>   /trunk/include/asterisk/dns_resolver.h PRE-CREATION 
>   /trunk/include/asterisk/dns_recurring.h PRE-CREATION 
>   /trunk/include/asterisk/dns_query_set.h PRE-CREATION 
>   /trunk/include/asterisk/dns_naptr.h PRE-CREATION 
>   /trunk/include/asterisk/dns_internal.h PRE-CREATION 
>   /trunk/include/asterisk/dns_core.h PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/4474/diff/
> 
> 
> Testing
> -------
> 
> Ran DNS unit tests as done by Mark, they are happy.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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