Report information
The Basics
Id:
45610
Status:
resolved
Priority:
Low/Low
Queue:
bind9-public
People
BugTracker
Version Fixed:
9.9.12, 9.10.7, 9.11.3, 9.12.0
Version Found:
(no value)
Versions Affected:
(no value)
Versions Planned:
(no value)
Priority:
P3 Low
Severity:
S3 Low
CVSS Score:
(no value)
CVE ID:
(no value)
Component:
BIND Infrastructure
Area:
test
Dates
Created:Tue, 25 Jul 2017 07:02:55 -0400
Updated:Tue, 01 Aug 2017 07:58:29 -0400
Closed:Tue, 01 Aug 2017 07:58:29 -0400

This bug tracker is no longer active.

Please go to our Gitlab to submit issues (both feature requests and bug reports) for active projects maintained by Internet Systems Consortium (ISC).

Due to security and confidentiality requirements, full access is limited to the primary maintainers.

History
  Tue, 25 Jul 2017 07:02:56 -0400 Michał Kępień <michal@isc.org> - Ticket created
To: bind9-public@isc.org
Subject: Refactor lib/dns/tests/rdata_test.c
Date: Tue, 25 Jul 2017 11:02:55 +0000
From: michal@isc.org

There is quite a lot of duplicate code in rdata_test.c that can be refactored to facilitate adding further tests.
  Tue, 25 Jul 2017 07:06:41 -0400 Michał Kępień <michal@isc.org> - Correspondence added

Refactoring was done in the rt45610 branch. Changes introduced: - conversions to/from type-specific structures were previously performed only as part of wire data processing tests for selected RR types; these conversions are now performed for all correctly processed input RDATA, both in text and wire form, - error_callback() and warn_callback() looked like placeholders; as dns_rdata_fromtext() accepts NULL for its callbacks parameters, I removed these two functions from rdata_test.c, - text RDATA tests used to initialize the source buffer using isc_buffer_init(); initialization is now rather performed using isc_buffer_constinit(), in order to enable constifying passed arrays of input data. With these cleanups in place, adding tests for a new RR type should boil down to defining test RDATA in text and/or wire form and calling a single function. Please review.
Tue, 25 Jul 2017 07:06:42 -0400 Michał Kępień <michal@isc.org> - Status changed from 'open' to 'review'
Tue, 25 Jul 2017 07:06:42 -0400 Michał Kępień <michal@isc.org> - Untaken
  Tue, 25 Jul 2017 15:30:25 -0400 Evan Hunt <each@isc.org> - Correspondence added
To: "Michal Kepien via RT" <bind9-public@isc.org>
CC:
From: "Evan Hunt" <each@isc.org>
Subject: Re: [ISC-Bugs #45610] Refactor lib/dns/tests/rdata_test.c
Date: Tue, 25 Jul 2017 19:30:22 +0000

On Tue, Jul 25, 2017 at 11:06:42AM +0000, Michal Kepien via RT wrote: > With these cleanups in place, adding tests for a new RR type should boil > down to defining test RDATA in text and/or wire form and calling a > single function. This is good. While you're in there, would you mind looking over bin/tests/rdata_test.c (an obsolete test from before we used ATF), and see whether there's anything useful in there that we can move to lib/dns/tests/rdata_test.c? If so, let's move the code over, then remove the file, and if not, let's just remove the file.
  Thu, 27 Jul 2017 03:16:08 -0400 Michał Kępień <michal@isc.org> - Correspondence added

Ray suggested that error reporting could also be improved. After I took another look at the branch, I noticed two bugs it introduced that flew under the radar. Those are now fixed (details are described in commit messages). Ray's suggestion is also implemented. In the process, I "ported" the -z switch from bin/tests/rdata_test.c (testing zero-length wire form RDATA). I will keep looking for interesting bits in that file and report back when I am done.
Thu, 27 Jul 2017 03:16:09 -0400 Michał Kępień <michal@isc.org> - Status changed from 'review' to 'open'
Thu, 27 Jul 2017 03:16:09 -0400 Michał Kępień <michal@isc.org> - Taken
  Fri, 28 Jul 2017 04:20:36 -0400 Michał Kępień <michal@isc.org> - Correspondence added

Further tweaks applied: - use macros to simplify test definitions and aid debugging (Ray), - updated outdated EDNS Client Subnet "source generic" test, - added RFC snippets for reference (Stephen). I went through bin/tests/rdata_test.c and did not find anything worth keeping: "-s" prints out memory stats; I could not think of a reasonable way to weave this into ATF-based tests; each unit test inside lib/dns/tests/rdata_test.c checks for unreleased memory, which is IMHO the required minimum; suggestions are welcome if anyone thinks we should somehow add memory stats to lib/dns/tests/rdata_test.c, "-t" passes a truncated buffer to dns_rdata_fromwire(), "-a" passes a buffer with trailing zero bytes; as expected results vary by RR type, we can just add explicit unit tests handling these cases, "-r" prints out RDATA read from stdin in uncompressed wire form; again, not sure how this would fit into ATF-based tests, it seems to be more of a debugging aid than a check. Given the above, I removed bin/tests/rdata_test.c in the final commit to the rt45610 branch. It is now ready for review (due to the sheer amount of changes, it probably makes most sense to review commit by commit).
Fri, 28 Jul 2017 04:20:37 -0400 Michał Kępień <michal@isc.org> - Status changed from 'open' to 'review'
Fri, 28 Jul 2017 04:20:37 -0400 Michał Kępień <michal@isc.org> - Untaken
  Fri, 28 Jul 2017 13:57:16 -0400 Evan Hunt <Evan_Hunt@isc.org> - Correspondence added

My only suggestion is that data_to_hex() looks pretty generically useful, so I'd put it into dnstest.c, and pass in a static buffer when calling instead of using isc_mem_allocate(). I pushed that change to the branch, if you're okay with it then I think this is fine to commit. (Note, I only evaluated the code, I haven't checked the test case data to confirm that it tests what it says it does. I take it that's unchanged from previous versions though.)
Fri, 28 Jul 2017 13:57:17 -0400 Evan Hunt <Evan_Hunt@isc.org> - Given to Michał Kępień <michal@isc.org>
  Tue, 01 Aug 2017 06:10:07 -0400 Michał Kępień <michal@isc.org> - Correspondence added

> My only suggestion is that data_to_hex() looks pretty generically useful, > so I'd put it into dnstest.c, and pass in a static buffer when calling instead > of using isc_mem_allocate(). > > I pushed that change to the branch, if you're okay with it then I think this > is fine to commit. Sure, thanks, your change looks good to me. > (Note, I only evaluated the code, I haven't checked the test case data to > confirm that it tests what it says it does. I take it that's unchanged from > previous versions though.) The only commit which changes test data is 02c3232b17 (and it is intentional).
Tue, 01 Aug 2017 07:58:28 -0400 Michał Kępień <michal@isc.org> - Version Fixed 9.9.12, 9.10.7, 9.11.3, 9.12.0 added
  Tue, 01 Aug 2017 07:58:28 -0400 Michał Kępień <michal@isc.org> - Correspondence added

4667. [cleanup] Refactor RDATA unit tests. [RT #45610] 9.9.12, 9.10.7, 9.11.3, 9.12.0 Cherry-picking to v9_9 and v9_9_sub was done without updating the "source generic" test and with the expected result for EDNS Client Subnet test "Option code family 0, source 0, scope 0" changed to ISC_FALSE. This is because the v9_9 branch lacks change 4468 ("Address ECS option handling issues. [RT #43191]") which changes the way the relevant wire data is processed. Change 4468 is present in all of v9_10, v9_10_sub and v9_11.
Tue, 01 Aug 2017 07:58:29 -0400 Michał Kępień <michal@isc.org> - Status changed from 'review' to 'resolved'