/team/group/dns/include/asterisk/dns_internal.h (Diff revision 2)

Doxygen comment.

In particular, the comment should explain the relationship of data_ptr to data, and why it is necessary.

Show all issues

/team/group/dns/include/asterisk/dns_internal.h (Diff revision 2)

Doxygen comment

Show all issues

/team/group/dns/main/dns_naptr.c (Diff revision 2)

The asserts here are appropriate.

However, if there is an error in the record, such that the memcmp never returns true, we'll get stuck in this loop.

It may be good to have a 'fail safe' break out in the loop, after the asserts. That way in dev-mode we'll catch issues, but in production systems, the allocation of the NAPTR record will fail and we can (hopefully) gracefully handle it.

Show all issues

1. Mark Michelson April 1, 2015, 8:55 a.m. (April 1, 2015, 8:55 a.m.)

   I'm not sure I agree with this.
   
   The reason why these are assertions instead of if statements is because the record we have been given came from the DNS answer in the first place, so it HAS to be present in the answer. Even if the answer is malformed in some way, the record we've been given comes from that answer, so it needs to be present.
   
   On the other hand, it's not difficult to add a fail-safe if check here just to be certain.

/team/group/dns/main/dns_naptr.c (Diff revision 2)

Suggestion: since this is repeated after each check, you may want to macro-tize it:

#define CHECK_END_OF_RECORD do { \
    if (ptr => end_of_record) { \
        return NULL; \
    } } while (0)


Then you can just put:

ptr += 2;
CHECK_END_OF_RECORD;

Or something like that.

Show all issues

1. rmudgett March 31, 2015, 10:36 a.m. (March 31, 2015, 10:36 a.m.)

   Doing this hides return points.  Which adds a potential for memory and ref leaks.

2. Matt Jordan March 31, 2015, 2:20 p.m. (March 31, 2015, 2:20 p.m.)

   True, right now all of these do not perform cleanup. In fact, the entire exercise here is mostly to figure out how long everything is so that things can be allocated.
   
   This is a long routine; sometimes it's nice to take repetitive code and squish it down. If Mark feels like that injects too much risk, I have no problem with the finding being dropped.
   

3. Mark Michelson April 1, 2015, 8:55 a.m. (April 1, 2015, 8:55 a.m.)

   Yeah, I'm not proud of this routine's length, and I wanted to break it up more than it already is, but unfortunately everything I came up with ended up either being uglier or questionable.
   
   The macro idea has some merit to it because it prevents fat-fingering and accidentally comparing the wrong values or typing a > instead of a >=. But as Richard pointed out, this hides return points, which in my opinion ultimately made the code less readable.
   
   I could make a compromise and have
   
   #define CHECK_END_OF_RECORD (ptr >= end_of_record)
   ...
   if (CHECK_END_OF_RECORD) {
       return NULL
   }
   
   Readability is a bit compromised, but the typo possibilities are eliminated. I'll just go with that.

/team/group/dns/main/dns_naptr.c (Diff revision 2)

I'd check to make sure num_records is non-zero before allocating the array.

If it is zero, you can simply bail out of the routine.

Show all issues

/team/group/dns/include/asterisk/dns_internal.h (Diff revision 2)

These methods should not start with ast_* unless they are meant to be exposed externally/exported. If they are then they should be moved to a more "public" include file.

Show all issues

/team/group/dns/main/dns_naptr.c (Diff revision 2)

This seems like it should be a non assert check. What happens if asterisk is not compiled without debug on and this is false?

Show all issues

1. Mark Michelson March 31, 2015, 12:05 p.m. (March 31, 2015, 12:05 p.m.)

   The reason this is an assertion is because it should never be false.
   
   Before regexp_repl_invalid is called, we divide the regexp based on the delimiter, and we ensure that we do not divide on backslash-escaped delimiters. This means that the regexp repl section cannot end with a backslash, unless it ends in a backslash-escaped backslash ("\\"). Current code would find the first backslash of the pair, and since the backslash is escaping an illegal character, the routine would return -1 before possibly finding the second backslash. If the code were altered to allow for backslash-escaped backslash, then the code would have to be altered from its current form and the assertion would need to be changed to a different type of check.

/team/group/dns/res/res_resolver_unbound.c (Diff revision 2)

Any reason to continue if a failure occurs?

Show all issues

1. Mark Michelson March 31, 2015, 12:05 p.m. (March 31, 2015, 12:05 p.m.)

   Since previous failures don't affect future operations, my thought was to go through with the whole thing so that we can see if everything is failing or only a specific case. Of course the status update messages here aren't very helpful for determining what specific failure was seen, though. I can address that though.

/team/group/dns/tests/test_dns_naptr.c (Diff revision 2)

Should these failures break the loop and just goto cleanup as well?

Show all issues

1. Mark Michelson March 31, 2015, 12:05 p.m. (March 31, 2015, 12:05 p.m.)

   See my previous comment. A failure of one record doesn't necessarily mean the failure of another. This way, all failures can be on display if the unit test fails for whatever reason.

Ship It!