Skip to content
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

Export EDNS0 interface #1041

Merged
merged 1 commit into from Dec 6, 2019
Merged

Export EDNS0 interface #1041

merged 1 commit into from Dec 6, 2019

Conversation

omeranson
Copy link
Contributor

@omeranson omeranson commented Dec 4, 2019

Replace all the private methods in the EDNS0 with public methods.
Additionally, as suggested in issue #857, made Pack receive a
pre-allocated byte array, introduce a Len method, and have Pack
and Unpack return the number of octets written and read (respectively)
if there was no error.

Closes #857

@codecov-io
Copy link

@codecov-io codecov-io commented Dec 4, 2019

Codecov Report

Merging #1041 into master will decrease coverage by 0.09%.
The diff coverage is 24.3%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1041     +/-   ##
=========================================
- Coverage   55.05%   54.95%   -0.1%     
=========================================
  Files          41       41             
  Lines        9879     9906     +27     
=========================================
+ Hits         5439     5444      +5     
- Misses       3419     3441     +22     
  Partials     1021     1021
Impacted Files Coverage Δ
ztypes.go 45.87% <0%> (ø) ⬆️
msg_helpers.go 58.46% <16.66%> (+0.09%) ⬆️
edns.go 27.38% <25.19%> (-0.37%) ⬇️
server.go 67.4% <0%> (-0.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22cda6d...0e87d06. Read the comment docs.

Copy link
Owner

@miekg miekg left a comment

thanks for picking this up, mostly looks OK.

edns.go Outdated
b := make([]byte, 4)
func (e *EDNS0_SUBNET) Len() int {
l := 4
switch e.Family {
Copy link
Owner

@miekg miekg Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think this can be more easily done with an if
if e.Family == 1 { return 4 } and then handle the Family == 2 case. Note sure what the RFC says about other families.

Copy link
Contributor Author

@omeranson omeranson Dec 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the RFC. It says

   The format of the address part depends on the value of FAMILY.  This
   document only defines the format for FAMILY 1 (IPv4) and FAMILY 2
   (IPv6), which are as follows:

Sounds to me like FAMILY only controls the format of ADDRESS, but SOURCE PREFIX-LENGTH and SCOPE PREFIX-LENGTH retain their meaning. In any case, only IPv4 and IPv6 are defined, to which this generalisation holds.

Therefore, I simplified the code to

return 4 + int((e.SourceNetmask + 8 - 1) / 8)
```

Replace all the private methods in the EDNS0 with public methods.
Additionally, as suggested in issue miekg#857, made Pack receive a
pre-allocated byte array, introduce a Len method, and have Pack
and Unpack return the number of octets written and read (respectively)
if there was no error.

Closes miekg#857
miekg
miekg approved these changes Dec 6, 2019
@miekg miekg merged commit a98e771 into miekg:master Dec 6, 2019
2 checks passed
miekg added a commit that referenced this issue Dec 6, 2019
This reverts commit a98e771.

This is breaking people
miekg added a commit that referenced this issue Dec 6, 2019
This reverts commit a98e771.

This is breaking people
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants