[Asterisk-code-review] DNS: Create a system-level DNS resolver (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: DNS: Create a system-level DNS resolver
......................................................................


Patch Set 7:

(12 comments)

https://gerrit.asterisk.org/#/c/749/7/include/asterisk/dns.h
File include/asterisk/dns.h:

Line 49:  * \note Asterisk DNS is synchronus at this time. This means that if your DNS
       :  *       services does not work, Asterisk may lock while waiting for response.
       :  
...your DNS service does...waiting for a response.


Line 55: /*! \brief Extended version of the DNS search function. Performs a DNS lookup, (used by
       :  *         DNS, enum and SRV lookups), parses the results and notifies the observer with the
       :  *         response and discovered records via invoking the provided callbacks (used by
       :  *         ast_dns_system_resolver
\brief should be a single line description of the function.  Hence the term brief instead of long winded. :)

\details is where you should add more detailed descriptions about the function.


Line 64: * \param  response_handler  Callback function for handling the DNS response. Invoked upon
       :                              completion of the DNS search.
Missing * at beginning of line.


Line 75: * \note Asterisk DNS is synchronus at this time. This means that if your DNS
       :  *       services does not work, Asterisk may lock while waiting for response.
...your DNS service does...waiting for a response.

Looks like copy-pasta of engrish typos. :)


https://gerrit.asterisk.org/#/c/749/7/main/dns.c
File main/dns.c:

Line 203: 	if (x >= len)
        : 		return AST_DNS_SEARCH_FAILURE;
You might as well add the missing curlies while you're here.


Line 275: 	return AST_DNS_SEARCH_FAILURE;     /* No search performed */
The conditional code is strange here.  This line is unreachable if the #ifndef HAVE_RES_NINIT is true.


Line 317: 	return AST_DNS_SEARCH_FAILURE;     /* No search performed */
Same here about unreachable line.


Line 463: 			ast_log(LOG_WARNING, "Length of DNS answer exceeds available search frames \n");
Extraneous space before \n.


Line 579: 		ast_log(LOG_WARNING, "DNS Parse error for %s\n", dname);                  /* Parsing Error */
        : 	} else if (ret == AST_DNS_SEARCH_NO_RECORDS) {
        : 		ast_log(LOG_WARNING, "DNS search yielded no results for %s\n", dname);    /* No results found */
1) Tail comments are over 90 columns.
IMHO: Tail comments in general are just about useless as there is little room to add any value without being criptic or restating what the code is saying.  In this case both tail comments are repeating the warning message contents.

It would be better if those comments were put on the line above:
/* Parsing Error */
ast_log(...);


2) The yield no results warning should be an ast_debug instead just like ast_search_dns().


https://gerrit.asterisk.org/#/c/749/7/main/dns_system_resolver.c
File main/dns_system_resolver.c:

Line 87: 	return ast_dns_resolver_add_record(query,                               /* The DNS query */
       : 	                                   ast_dns_query_get_rr_type(query),    /* Resource record type */
       : 	                                   ast_dns_query_get_rr_class(query),   /* Resource record class */
       : 	                                   ttl,                                 /* TTL of the record */
       : 	                                   (const char*) record,                /* The raw DNS record */
       : 	                                   record_len);                         /* The size of the raw DNS record */
These tail comments go over column 90 because of the toothbrush formating.  If you only allow indention by one level for line continuation you would not go beyond 90 as easily.


Line 126: 	return;
This return is redundant.  It is the end of the function and it is a void function.


Line 157: 		ast_log(LOG_ERROR, "DNS search failed to yield any results for query: '%s'\n",
        : 		        ast_dns_query_get_name(query));
Is this really an error?


-- 
To view, visit https://gerrit.asterisk.org/749
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b36ea17b889a98df4f8d80d50bb7ee175afa077
Gerrit-PatchSet: 7
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes