BIND 10 #2095: Define and implement RdataEncoder class
BIND 10 Development
do-not-reply at isc.org
Mon Jul 30 20:06:31 UTC 2012
#2095: Define and implement RdataEncoder class
-------------------------------------+-------------------------------------
Reporter: | Owner: jinmei
jinmei | Status: reviewing
Type: task | Milestone:
Priority: | Sprint-20120731
medium | Resolution:
Component: data | Sensitive: 0
source | Sub-Project: DNS
Keywords: | Estimated Difficulty: 5
Defect Severity: N/A | Total Hours: 0
Feature Depending on Ticket: |
scalable inmemory |
Add Hours to Ticket: 0 |
Internal?: 0 |
-------------------------------------+-------------------------------------
Comment (by jinmei):
Replying to [comment:12 vorner]:
Thanks for the review.
> It doesn't compile for me. I think I saw this somewhere and it is maybe
because you based it on another branch and it should be fixed in master,
but a merge with master has conflicts:
Ah, right. I've cherry-picked the corresponding fix.
> Some minor comments:
> * The exception text „RDATA encoder finds missing field„ looks slightly
strange to me. If it is missing, then it can't be found.
Tried to clarify it. Is it okay now?
> * The code in `updateOtherData` is confusing. I'm not really sure how
to improve that, maybe placing the variable length thing into an else
branch instead of using continue could be an option.
I agree it's not very understandable. In fact I didn't really like
that implementation either. On closer look I found it could be much
more simplified, and I did it at commit 8560883.
> Also, I'd like to see the thing does not go outside the buffer that is
passed to it. Would it be possible to do a test with a canary vale after
the data and see it survived?
you mean as a result of encode()? I think the assert() check would
catch any such bugs, but I've added some explicit check in the
unittests with canary data (commit 84e7463).
--
Ticket URL: <http://bind10.isc.org/ticket/2095#comment:13>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list