[asterisk-dev] [Code Review] 4528: dns: Add SRV recording parsing, sorting/weighting, and unit tests

[Mark Michelson]

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



/trunk/main/dns_srv.c
<https://reviewboard.asterisk.org/r/4528/#comment25587>

    Let's talk alignment.
    
    In DNS, there is no padding used for variable-length elements. There is no guarantee that RRs (or their RDATA) lie on word-boundaries. This means that the "data" pointer passed to ast_dns_srv_alloc() may be pointing to data that is not word-aligned (or not even on a half-word).
    
    So in this case, you've overlaid a byte array on top of a struct that contains 16-bit unsigned integers. Typically, 16-bit values should be aligned on word or half-word boundaries, but because of the data layout of the DNS record, this cannot be guaranteed. This means you may be performing some unaligned reads here.
    
    Now the question is, how bad is this? I'm pretty sure that on x86 architectures, an unaligned read isn't a big deal, performance-wise, but I can't make any claims for other architectures simply because I don't know. Can someone more familiar with these sorts of things chime in with some info?



/trunk/main/dns_srv.c
<https://reviewboard.asterisk.org/r/4528/#comment25593>

    Reading the language of RFC 2782, I'm unclear if what you're doing here is actually correct:
    
    "To select a target to be contacted next, arrange all SRV RRs (that have not been ordered yet) in any order, except that all those with weight 0 are placed at the beginning of the list."
    
    I can interpret this two different ways:
    
    1) Select all records of a given priority, and place them in a set, ensuring that the zero-weight records are at the beginning of the set. Then run the crazy weighting algorithm.
    
    2) Select all records of a given priority. Take all 0-weighted records and place them into the sorted list of records first. Then run the crazy weighting algorithm on the rest of the records to determine their place in the list after the 0-weighted records.
    
    After re-reading a few times, I *think* they mean to do #1. If that's the case, then between the first highlighted list traversal and the while loop, you should place all 0-weight records at the front of temp_list. If they mean #2, then between the first highlighted list traversal and hte while loop, you should remove all 0-weighted records from temp_list and append them to the end of new_list.



/trunk/tests/test_dns_srv.c
<https://reviewboard.asterisk.org/r/4528/#comment25594>

    A single red blob.


- Mark Michelson


On March 26, 2015, 6:50 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4528/
> -----------------------------------------------------------
> 
> (Updated March 26, 2015, 6:50 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This change adds the following:
> 
> 1. As SRV records are added to a result the information is parsed and stored away in additional storage in the record. The SRV API can then be used to return this information.
> 2. Before invoking the DNS query callback the list of records on the result are sorted based on priority and weight.
> 3. Unit tests have been added which verify the record parsing, sorting, and weighting. There are also some off nominal which cover the cases when an invalid/corrupt record is received.
> 4. A unit test has also been added to res_resolver_unbound which adds an SRV record to a zone and confirms it is retrieved and parsed correctly.
> 
> 
> Diffs
> -----
> 
>   /trunk/tests/test_dns_srv.c PRE-CREATION 
>   /trunk/res/res_resolver_unbound.c 433370 
>   /trunk/main/dns_srv.c 433370 
>   /trunk/main/dns_core.c 433370 
>   /trunk/include/asterisk/dns_internal.h 433370 
> 
> Diff: https://reviewboard.asterisk.org/r/4528/diff/
> 
> 
> Testing
> -------
> 
> Executed unit tests and confirmed they pass.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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