Re: [asterisk-dev] [Code Review] 4542: DNS: Add NAPTR support and tests
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4542/ --- (Updated April 6, 2015, 12:06 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 434068 Repository: Asterisk Description --- This adds NAPTR support for DNS in Asterisk. The main parts of this are the functions for allocating a DNS NAPTR record when a resolver wishes to add a NAPTR record, the sorting algorithm for sorting DNS NAPTR records, and the tests that use a mock DNS resolver. NOTE: There is likely to be some overlap here in this review and Josh's SRV review (/r/4528). Our stance on this is that we will factor out the duplicated code once both SRV and NAPTR have been merged into the main DNS branch. The factoring out of common functions will be placed in its own review. Diffs - /team/group/dns/tests/test_dns_naptr.c PRE-CREATION /team/group/dns/res/res_resolver_unbound.c 433885 /team/group/dns/main/dns_naptr.c 433885 /team/group/dns/main/dns_core.c 433885 /team/group/dns/include/asterisk/dns_internal.h 433885 Diff: https://reviewboard.asterisk.org/r/4542/diff/ Testing --- All previous DNS tests continue to pass, and all new tests added in this review pass as well. Thanks, Mark Michelson -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4542: DNS: Add NAPTR support and tests
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4542/#review15031 --- Ship it! Ship It! - Kevin Harwell On April 1, 2015, 9:51 a.m., Mark Michelson wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4542/ --- (Updated April 1, 2015, 9:51 a.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- This adds NAPTR support for DNS in Asterisk. The main parts of this are the functions for allocating a DNS NAPTR record when a resolver wishes to add a NAPTR record, the sorting algorithm for sorting DNS NAPTR records, and the tests that use a mock DNS resolver. NOTE: There is likely to be some overlap here in this review and Josh's SRV review (/r/4528). Our stance on this is that we will factor out the duplicated code once both SRV and NAPTR have been merged into the main DNS branch. The factoring out of common functions will be placed in its own review. Diffs - /team/group/dns/tests/test_dns_naptr.c PRE-CREATION /team/group/dns/res/res_resolver_unbound.c 433885 /team/group/dns/main/dns_naptr.c 433885 /team/group/dns/main/dns_core.c 433885 /team/group/dns/include/asterisk/dns_internal.h 433885 Diff: https://reviewboard.asterisk.org/r/4542/diff/ Testing --- All previous DNS tests continue to pass, and all new tests added in this review pass as well. Thanks, Mark Michelson -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4542: DNS: Add NAPTR support and tests
On March 31, 2015, 3:25 p.m., Matt Jordan wrote:
/team/group/dns/main/dns_naptr.c, lines 420-421
https://reviewboard.asterisk.org/r/4542/diff/2/?file=73012#file73012line420
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.
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.
On March 31, 2015, 3:25 p.m., Matt Jordan wrote:
/team/group/dns/main/dns_naptr.c, lines 447-449
https://reviewboard.asterisk.org/r/4542/diff/2/?file=73012#file73012line447
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.
rmudgett wrote:
Doing this hides return points. Which adds a potential for memory and
ref leaks.
Matt Jordan wrote:
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.
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.
- Mark
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/#review14972
---
On March 27, 2015, 2:45 p.m., Mark Michelson wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/
---
(Updated March 27, 2015, 2:45 p.m.)
Review request for Asterisk Developers.
Repository: Asterisk
Description
---
This adds NAPTR support for DNS in Asterisk.
The main parts of this are the functions for allocating a DNS NAPTR record
when a resolver wishes to add a NAPTR record, the sorting algorithm for
sorting DNS NAPTR records, and the tests that use a mock DNS resolver.
NOTE: There is likely to be some overlap here in this review and Josh's SRV
review (/r/4528). Our stance on this is that we will factor out the
duplicated code once both SRV and NAPTR have been merged into the main DNS
branch. The factoring out of common functions will be placed in its own
review.
Diffs
-
/team/group/dns/tests/test_dns_naptr.c PRE-CREATION
/team/group/dns/res/res_resolver_unbound.c 433573
/team/group/dns/main/dns_naptr.c 433573
/team/group/dns/main/dns_core.c 433573
/team/group/dns/include/asterisk/dns_internal.h 433573
Diff: https://reviewboard.asterisk.org/r/4542/diff/
Testing
---
All previous DNS tests continue to pass, and all new tests added in this
review pass as well.
Thanks,
Mark Michelson
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4542: DNS: Add NAPTR support and tests
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4542/ --- (Updated April 1, 2015, 2:51 p.m.) Review request for Asterisk Developers. Changes --- Addressed review feedback from Matt and Kevin. * Added Doxygen comments for NAPTR data and DNS record data_ptr * Added safety checks to return NULL if NAPTR record not found in DNS result. * Macrotized the check if we are past the end of the NAPTR record when parsing. * Short-circuit the sort operation if fewer than two NAPTR records are returned * Removed ast_ prefix from internal DNS routines. * Error messages for nominal tests are more descriptive Repository: Asterisk Description --- This adds NAPTR support for DNS in Asterisk. The main parts of this are the functions for allocating a DNS NAPTR record when a resolver wishes to add a NAPTR record, the sorting algorithm for sorting DNS NAPTR records, and the tests that use a mock DNS resolver. NOTE: There is likely to be some overlap here in this review and Josh's SRV review (/r/4528). Our stance on this is that we will factor out the duplicated code once both SRV and NAPTR have been merged into the main DNS branch. The factoring out of common functions will be placed in its own review. Diffs (updated) - /team/group/dns/tests/test_dns_naptr.c PRE-CREATION /team/group/dns/res/res_resolver_unbound.c 433885 /team/group/dns/main/dns_naptr.c 433885 /team/group/dns/main/dns_core.c 433885 /team/group/dns/include/asterisk/dns_internal.h 433885 Diff: https://reviewboard.asterisk.org/r/4542/diff/ Testing --- All previous DNS tests continue to pass, and all new tests added in this review pass as well. Thanks, Mark Michelson -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4542: DNS: Add NAPTR support and tests
On March 31, 2015, 10:25 a.m., Matt Jordan wrote:
/team/group/dns/main/dns_naptr.c, lines 447-449
https://reviewboard.asterisk.org/r/4542/diff/2/?file=73012#file73012line447
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.
Doing this hides return points. Which adds a potential for memory and ref
leaks.
- rmudgett
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/#review14972
---
On March 27, 2015, 9:45 a.m., Mark Michelson wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/
---
(Updated March 27, 2015, 9:45 a.m.)
Review request for Asterisk Developers.
Repository: Asterisk
Description
---
This adds NAPTR support for DNS in Asterisk.
The main parts of this are the functions for allocating a DNS NAPTR record
when a resolver wishes to add a NAPTR record, the sorting algorithm for
sorting DNS NAPTR records, and the tests that use a mock DNS resolver.
NOTE: There is likely to be some overlap here in this review and Josh's SRV
review (/r/4528). Our stance on this is that we will factor out the
duplicated code once both SRV and NAPTR have been merged into the main DNS
branch. The factoring out of common functions will be placed in its own
review.
Diffs
-
/team/group/dns/tests/test_dns_naptr.c PRE-CREATION
/team/group/dns/res/res_resolver_unbound.c 433573
/team/group/dns/main/dns_naptr.c 433573
/team/group/dns/main/dns_core.c 433573
/team/group/dns/include/asterisk/dns_internal.h 433573
Diff: https://reviewboard.asterisk.org/r/4542/diff/
Testing
---
All previous DNS tests continue to pass, and all new tests added in this
review pass as well.
Thanks,
Mark Michelson
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4542: DNS: Add NAPTR support and tests
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4542/#review14984 --- /team/group/dns/include/asterisk/dns_internal.h https://reviewboard.asterisk.org/r/4542/#comment25605 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. /team/group/dns/main/dns_naptr.c https://reviewboard.asterisk.org/r/4542/#comment25604 This seems like it should be a non assert check. What happens if asterisk is not compiled without debug on and this is false? /team/group/dns/res/res_resolver_unbound.c https://reviewboard.asterisk.org/r/4542/#comment25608 Any reason to continue if a failure occurs? /team/group/dns/tests/test_dns_naptr.c https://reviewboard.asterisk.org/r/4542/#comment25606 Should these failures break the loop and just goto cleanup as well? - Kevin Harwell On March 27, 2015, 9:45 a.m., Mark Michelson wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4542/ --- (Updated March 27, 2015, 9:45 a.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- This adds NAPTR support for DNS in Asterisk. The main parts of this are the functions for allocating a DNS NAPTR record when a resolver wishes to add a NAPTR record, the sorting algorithm for sorting DNS NAPTR records, and the tests that use a mock DNS resolver. NOTE: There is likely to be some overlap here in this review and Josh's SRV review (/r/4528). Our stance on this is that we will factor out the duplicated code once both SRV and NAPTR have been merged into the main DNS branch. The factoring out of common functions will be placed in its own review. Diffs - /team/group/dns/tests/test_dns_naptr.c PRE-CREATION /team/group/dns/res/res_resolver_unbound.c 433573 /team/group/dns/main/dns_naptr.c 433573 /team/group/dns/main/dns_core.c 433573 /team/group/dns/include/asterisk/dns_internal.h 433573 Diff: https://reviewboard.asterisk.org/r/4542/diff/ Testing --- All previous DNS tests continue to pass, and all new tests added in this review pass as well. Thanks, Mark Michelson -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4542: DNS: Add NAPTR support and tests
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/#review14972
---
/team/group/dns/include/asterisk/dns_internal.h
https://reviewboard.asterisk.org/r/4542/#comment25588
Doxygen comment.
In particular, the comment should explain the relationship of data_ptr to
data, and why it is necessary.
/team/group/dns/include/asterisk/dns_internal.h
https://reviewboard.asterisk.org/r/4542/#comment25589
Doxygen comment
/team/group/dns/main/dns_naptr.c
https://reviewboard.asterisk.org/r/4542/#comment25591
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.
/team/group/dns/main/dns_naptr.c
https://reviewboard.asterisk.org/r/4542/#comment25592
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.
/team/group/dns/main/dns_naptr.c
https://reviewboard.asterisk.org/r/4542/#comment25590
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.
- Matt Jordan
On March 27, 2015, 9:45 a.m., Mark Michelson wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/
---
(Updated March 27, 2015, 9:45 a.m.)
Review request for Asterisk Developers.
Repository: Asterisk
Description
---
This adds NAPTR support for DNS in Asterisk.
The main parts of this are the functions for allocating a DNS NAPTR record
when a resolver wishes to add a NAPTR record, the sorting algorithm for
sorting DNS NAPTR records, and the tests that use a mock DNS resolver.
NOTE: There is likely to be some overlap here in this review and Josh's SRV
review (/r/4528). Our stance on this is that we will factor out the
duplicated code once both SRV and NAPTR have been merged into the main DNS
branch. The factoring out of common functions will be placed in its own
review.
Diffs
-
/team/group/dns/tests/test_dns_naptr.c PRE-CREATION
/team/group/dns/res/res_resolver_unbound.c 433573
/team/group/dns/main/dns_naptr.c 433573
/team/group/dns/main/dns_core.c 433573
/team/group/dns/include/asterisk/dns_internal.h 433573
Diff: https://reviewboard.asterisk.org/r/4542/diff/
Testing
---
All previous DNS tests continue to pass, and all new tests added in this
review pass as well.
Thanks,
Mark Michelson
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4542: DNS: Add NAPTR support and tests
On March 31, 2015, 10:25 a.m., Matt Jordan wrote:
/team/group/dns/main/dns_naptr.c, lines 447-449
https://reviewboard.asterisk.org/r/4542/diff/2/?file=73012#file73012line447
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.
rmudgett wrote:
Doing this hides return points. Which adds a potential for memory and
ref leaks.
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.
- Matt
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/#review14972
---
On March 27, 2015, 9:45 a.m., Mark Michelson wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4542/
---
(Updated March 27, 2015, 9:45 a.m.)
Review request for Asterisk Developers.
Repository: Asterisk
Description
---
This adds NAPTR support for DNS in Asterisk.
The main parts of this are the functions for allocating a DNS NAPTR record
when a resolver wishes to add a NAPTR record, the sorting algorithm for
sorting DNS NAPTR records, and the tests that use a mock DNS resolver.
NOTE: There is likely to be some overlap here in this review and Josh's SRV
review (/r/4528). Our stance on this is that we will factor out the
duplicated code once both SRV and NAPTR have been merged into the main DNS
branch. The factoring out of common functions will be placed in its own
review.
Diffs
-
/team/group/dns/tests/test_dns_naptr.c PRE-CREATION
/team/group/dns/res/res_resolver_unbound.c 433573
/team/group/dns/main/dns_naptr.c 433573
/team/group/dns/main/dns_core.c 433573
/team/group/dns/include/asterisk/dns_internal.h 433573
Diff: https://reviewboard.asterisk.org/r/4542/diff/
Testing
---
All previous DNS tests continue to pass, and all new tests added in this
review pass as well.
Thanks,
Mark Michelson
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4542: DNS: Add NAPTR support and tests
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4542/ --- Review request for Asterisk Developers. Repository: Asterisk Description --- This adds NAPTR support for DNS in Asterisk. The main parts of this are the functions for allocating a DNS NAPTR record when a resolver wishes to add a NAPTR record, the sorting algorithm for sorting DNS NAPTR records, and the tests that use a mock DNS resolver. NOTE: There is likely to be some overlap here in this review and Josh's SRV review (/r/4528). Our stance on this is that we will factor out the duplicated code once both SRV and NAPTR have been merged into the main DNS branch. The factoring out of common functions will be placed in its own review. Diffs - /team/group/dns/tests/test_dns_naptr.c PRE-CREATION /team/group/dns/res/res_resolver_unbound.c 433573 /team/group/dns/main/dns_naptr.c 433573 /team/group/dns/main/dns_core.c 433573 /team/group/dns/include/asterisk/dns_internal.h 433573 Diff: https://reviewboard.asterisk.org/r/4542/diff/ Testing --- All previous DNS tests continue to pass, and all new tests added in this review pass as well. Thanks, Mark Michelson -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4542: DNS: Add NAPTR support and tests
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4542/ --- (Updated March 27, 2015, 2:45 p.m.) Review request for Asterisk Developers. Changes --- Uncomment a test case from the nominal NAPTR test in test_dns_naptr.c Repository: Asterisk Description --- This adds NAPTR support for DNS in Asterisk. The main parts of this are the functions for allocating a DNS NAPTR record when a resolver wishes to add a NAPTR record, the sorting algorithm for sorting DNS NAPTR records, and the tests that use a mock DNS resolver. NOTE: There is likely to be some overlap here in this review and Josh's SRV review (/r/4528). Our stance on this is that we will factor out the duplicated code once both SRV and NAPTR have been merged into the main DNS branch. The factoring out of common functions will be placed in its own review. Diffs (updated) - /team/group/dns/tests/test_dns_naptr.c PRE-CREATION /team/group/dns/res/res_resolver_unbound.c 433573 /team/group/dns/main/dns_naptr.c 433573 /team/group/dns/main/dns_core.c 433573 /team/group/dns/include/asterisk/dns_internal.h 433573 Diff: https://reviewboard.asterisk.org/r/4542/diff/ Testing --- All previous DNS tests continue to pass, and all new tests added in this review pass as well. Thanks, Mark Michelson -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
