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

[Mark Michelson]

Mark Michelson has posted comments on this change.

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


Patch Set 3: Code-Review-1

(8 comments)

This looks mostly good. Aside from the dns_update_frame() issue, most of what I'm commenting on is style-related or just based on personal taste.

One trend I notice in this code is that it seems a bit over-commented to me. For oddball operations like dns_parse_answer_ex(), comments for each section are good. However, comments like "unreference the taskprocessor" above a call to ast_taskprocessor_unreference() are superfluous.

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

Line 51: /*! \brief Return code upon failure. */
       : #define DNS_SEARCH_FAILURE -1
       : 
       : /*! \brief Return code upon no records found. */
       : #define DNS_SEARCH_NO_RECORDS 0
       : 
       : /*! \brief Return code upon success. */
       : #define DNS_SEARCH_SUCCESS 1
I think these return codes should be

1) Placed into an enum
2) Made public

This way, ast_search_dns_ex() would return an enum ast_search_dns_result (or whatever it gets called), which is much more indicative than an int return of what the possible values will be.


Line 225: * \param  dns_response   A char pointer to the current frame in the DNS response.
        :  * \param  remaining_len  The remaining available length in the DNS response to search.
The frame_size parameter is not documented here.


Line 233: 	dns_response += frame_size;
This line has a flaw in it. You're expecting the caller to have its local pointer updated by this function. Unfortunately, all this line is going to do is update the copy of the pointer in this function instead. I think the way to go is to pass a pointer to pointer into this function instead.


Line 470: 		/* Try updating the cursors for the current record */
        : 		if ((pos = dns_update_frame(answer, pos, res + sizeof(struct dn_answer)))  < 0) {
        : 			return DNS_SEARCH_FAILURE;
        : 		}
        : 
        : 		/* Cast the current value for the answer pointer as a dn_answer struct */
        : 		ans = (struct dn_answer *)answer;
I think these two blocks need to be reversed. In other words, set ans first, then update the frame. Otherwise, ans will be pointing at something that should not be cast to a dn_answer.

(Technically, this cast is pretty bad, alignment-wise, but since we're piggybacking on old code that didn't care, I'm not going to gripe about that).


Line 481: 			if (record_handler) {
        : 				record_handler(context, answer, ntohs(ans->size), ans->ttl);
        : 				ret = DNS_SEARCH_SUCCESS;
        : 			}
This is good defensive code, but I would say that record_handler had better be non-NULL. Otherwise, it's pretty pointless to be calling this DNS function in the first place. I suggest that rather than catering to a NULL value here, I would place assertions in ast_search_dns_ex() that the two callbacks passed in are non-NULL. Then here in this function, you can call into those callbacks with no worries of them being NULL.


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

Line 91: 	                                   (const char*)record,                 /* The raw DNS record */
Nitpick, but space between the cast and the variable name.


Line 185: 	int res = ast_taskprocessor_push(dns_system_resolver_tp,
        : 	                                 dns_system_resolver_process_query,
        : 	                                 ao2_bump(query));
        : 
        : 	/* The query processing handler was not added to the task processor */
        : 	if (res == DNS_SYSTEM_RESOLVER_FAILURE) {
Checking res against DNS_SYSTEM_RESOLVER_FAILURE here seems odd considering that you are checking a return code from something outside of the DNS system resolver.


Line 224: 		                                  (const char*)dns_response,      /* The raw DNS answer */
Nitpick, but space between the cast and the variable name.


-- 
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: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes