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

[Ashley Sanders]

Ashley Sanders has posted comments on this change.

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


Patch Set 5:

(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:
Done.


Line 73: int ast_search_dns_ex(void *context, const char *dname, int rr_class, int rr_type,
> Sorry if I was unclear earlier with one of my suggestions, but in addition 
So, I don't disagree with you. The following are just devil's advocate points that I am throwing out there to further the discussion...

A very brief cursory search of Asterisk produced results where most of the time, we returned an int from the function, even though the implementation returned a public-facing enum. For reference, look at module.h and some of its concrete implementations of load and unload.

Also, it might prove advantageous to have a shared convention between the search functions such that the return types do not differ (in order to have consistency in the API).


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 t
I agree with you. Although this was already declared in _private.h, I was getting compiler warnings when I did not have it. Since I have enabled dev mode, warnings are treated as errors.

I looked to some other files that also provided their entry-point functions inside of that file (e.g. http.h/http.c) and could not determine the reason mine was failing.

Thoughts? I would love to remove this if possible.


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 ser
Done. The change will be available in the next revision.


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 avail
If I understand you correctly, that is already present below (see the ast_search_dns fn, line 496). 

My contention with that approach is it is incredibly difficult for the next person to read due to all the clutter the conditional groups create. I think this way, with each function representing only the part for which it is responsible, is a bit cleaner and thus easier to follow.


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
Good point. That will be addressed with the next revision.


-- 
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