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

[Joshua Colp]

Joshua Colp has posted comments on this change.

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


Patch Set 5: Code-Review-1

(6 comments)

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

Line 56:  *         DNS, enum and SRV lookups), parses the results and notifies observers of any
There can't be multiple observers, I'd reword this as a result:

invokes a callback with each record


https://gerrit.asterisk.org/#/c/749/5/include/asterisk/dns_system_resolver.h
File include/asterisk/dns_system_resolver.h:

Line 32: int ast_dns_system_resolver_init(void);
Does this file need to exist at all? You forward declared the presence of this function in _private.h


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

Line 48: /*! \brief Size used for creating the DNS response container. */
This is actually the maximum size permitted for the answer from the DNS server


Line 250: static int dns_search_res_n(const char *dname, int rr_class, int rr_type,
Instead of having two functions with different names which are always available and trying both (one of which will fail) why not have one function but two implementations, and depending on the available functionality only one of them is compiled in?


Line 564: 	/* Assert that the callbacks are not NULL */
        : 	ast_assert(response_handler != NULL);
        : 	ast_assert(record_handler != NULL);
I'd assert sooner and not even do the DNS lookup


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

Line 216: 	if (!ast_dns_query_get_result(query)) {
        : 		res = ast_dns_resolver_set_result(query,                          /* The DNS query */
        : 		                                  0,                              /* Is the result secure? */
        : 		                                  0,                              /* Is the result bogus? */
        : 		                                  rcode,                          /* Optional response code */
        : 		                                  ast_dns_query_get_name(query),  /* Canonical name */
        : 		                                  (const char*) dns_response,     /* The raw DNS answer */
        : 		                                  dns_response_len);              /* The size of the raw DNS answer */
At this point in time there won't be a result ever, only if either:

1. The basic DNS stuff in the core calls into us more than once on a response (which it shouldn't)
2. Core DNS sets result information (which it can't)

If either of those occur then the constraints of the environment has changed and that's bad.


-- 
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: 5
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-HasComments: Yes