-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expand DNAME support #170
Expand DNAME support #170
Conversation
- Allow access to DNAME RR - Add unitary test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
lib/Zonemaster/LDNS/RR/DNAME.pm
Outdated
=cut | ||
=item dname() | ||
|
||
Returns the delegation name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is clearer to say the RDATA of a DNAME record.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
MODULE = Zonemaster::LDNS PACKAGE = Zonemaster::LDNS::RR::DNAME PREFIX=rr_dname_ | ||
|
||
char * | ||
rr_dname_dname(obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: name the method target
instead, as it seems to be the given name of the RDATA RFC 6672 section 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering how we use such methods in Engine, I would prefer to keep them this way. However I have clarified the documentation to account for that technicality. Is that fine for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I understand. This is totally fine then.
- Clarify documentation of method 'Zonemaster::LDNS::RR::DNAME::dname()'
=cut | ||
=item dname() | ||
|
||
Returns the delegation name, i.e. the <target> field from the RDATA of a DNAME record. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RDATA only consists of the target so "from the RDATA" sounds strange. There is only one field to fetch. Maybe
Returns the delegation name, i.e. the DNAME record target (RDATA).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a comment, but not very important.
Purpose
This PR expands the functionality of DNAME resource records by giving access to its RDATA.
Context
Necessary for zonemaster/zonemaster-engine#568
Relates to zonemaster/zonemaster#1075 and zonemaster/zonemaster#472
Changes
How to test this PR
Unit tests should pass