[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 3:

(7 comments)

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
Okay. I can do that for the next revision.


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.
Done. Thanks!


Line 233: 	dns_response += frame_size;
> This line has a flaw in it. You're expecting the caller to have its local p
You're right. I saw this earlier and made a mental note to go back and update it and then forgot about the mental note. Thank you for catching this.


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
Ahh, you're right. I missed something. This, according to lines 378-386 was supposed to:
1. Add the res variable (the result of skipping to the next field in the DNS response) to the pos variable
2. Reduce the value of len by res
3. Assign ans the value of answer cast as a struct dn_answer pointer.
4. Add the size of a dn_answer struct to the pos variable.
5. Reduce the value of len by the size of a dn_answer struct.

I erroneously combined 1, 4 and 2, 5 into one command. 

Thank you. Very good catch.


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
Okay. That sounds reasonable.

Regarding my thoughts on the topic of defensive programming:

Since I have been dinged on this point in a few reviews, I did some reading on defensive versus offensive programming this evening and this is what I have concluded:

For compliance to internal contracts, offensive programming is in most cases best. This is especially applicable for things that developers will care about (especially if you can provide a good stack trace) - like internal API contract compliance and unit and integration testing.

When dealing with end-user input, defensive programming seems appropriate, or when dealing with real-time applications where a limping system would be a better alternative than one that falls on its own sword.

I am still reading about the differing philosophies (offensive versus defensive) but perhaps a discussion on would not be a bad thing.

I absolutely realize that you can take defensive programming entirely too far where you are now carrying weight of unnecessary things like null checks. My thoughts with the null check above were based on the original code (see line 389). Based on that line, I had the idea that it was optional to supply the callback and ergo checking for null was appropriate. However, I agree with you that the assert is a much better approach than a null check.

This brings me to a couple of points of contention I have with the original code: its Fail Fast nature and the point at where defensiveness is applied. 

Regarding point #1, if any one callback results in a parsing error, the function immediately terminates (see line 391). That seems extreme to me because it provides very little useful feedback to the API consumer. It seems a better strategy to finish parsing the DNS response for DNS records, log all errors, then return the failure code.

Regarding point #2, refer to line 389 (and by extension line 491). If the entire point of parsing a record is irrelevant when no callback is provided, then why start parsing at all? You just wasted precious processing cycles to yield absolutely no output. If we want to be defensive with a null check but remain useful, the null check should happen way before any parsing occurs, sort of like the response_handler null check on line 436, or before the function is ever called in the first place, with an assert, as Mark suggested.

P.S. I apologize for the novella.


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


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


-- 
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: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes