BIND 10 #2522: support generic version of rdata::createRdata(text) in RP, MINFO, TSIG RDATA
BIND 10 Development
do-not-reply at isc.org
Thu May 23 22:15:37 UTC 2013
#2522: support generic version of rdata::createRdata(text) in RP, MINFO, TSIG
RDATA
-------------------------------------+-------------------------------------
Reporter: jinmei | Owner:
Type: task | jinmei
Priority: medium | Status:
Component: libdns++ | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-20130528
Sub-Project: DNS | Resolution:
Estimated Difficulty: 5 | CVSS Scoring:
Total Hours: 0 | Defect Severity: N/A
| Feature Depending on Ticket:
| loadzone-ng
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Comment (by jinmei):
Replying to [comment:22 pselkirk]:
> > '''rdata_tsig_unittest.cc'''
> >
> > - multi line case doesn't seem to be tested either (also for others)
>
> I've only been adding multi-line tests for rdata types with terminal
key-data fields where spaces are allowed in the key data (dhcid, rrsig,
sshfp), in order to test multi-line keys.
>
> RFC2845 doesn't specify a presentation format, so we've defined the data
fields as base-64 with no spaces (or line breaks, as I tested it out). It
appears that BIND 9 also doesn't allow whitespace of any sort in the MAC
and Other Data fields.
>
> In any case, I added a test for multi-line ordinary data, which could be
done for any rdata type with multiple fields. Is this what you mean by
"also for others" - that all rdata unit tests should have a multi-line
test?
In general, I'll try to cover all general cases for completeness, but
in this particular case I could live with skipping this check, because
TSIG won't appear in a zone file and it'd be very unlikely to see
multi-line input in practice. I guess this was more about the review
process protocol: if you are silent about a point, it's not clear to
me whether you overlooked it or decided to not address it. Sometimes
it might be obvious, but in general it would be better to be explicit.
As for "also for others"...I don't remember what I meant:-) but
it probably meant other Rdata classes updated in this branch (and I
guess I then added the same comments for them, which might make it
confusing). In any case, I don't think it's an open issue anymore.
> > '''sshfp_44.cc'''
> > - [if you switch to this type of pimpl, you'll need to define
> > `operator=()`; otherwise it would eventually cause duplicate free.]
> > (check that by writing a test)
> >
> > For the latter, maybe I was not clear but I intended to suggest adding
> > a unit test case for assignment. See, e.g., `Rdata_HINFO_Test.copy`.
>
> Added. My oversight.
The added test actually uses the copy constructor, not `operator=`.
I've updated the test with a suggested test that uses the latter.
With this change (assuming it's okay), please merge.
--
Ticket URL: <http://bind10.isc.org/ticket/2522#comment:23>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list