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

Restrict option parsers to their compatible options #487

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

hugelgupf
Copy link
Collaborator

@hugelgupf hugelgupf commented Feb 19, 2023

  • Top-level messages can only parse options that belong to top-level messages.
  • Relay messages can only parse options that belong to relay messages.
  • IAPD can parse IAPrefix + StatusCode
  • IATA/IANA can parse IAAddress + StatusCode
  • etc etc

All other options are still parsed; the option parser does not return an error. Most RFCs that specify options say that one should ignore options specified in places they shouldn't be, rather than take other actions.

This also moves us toward satisfying requests like #395 -- being able to implement lazy parsing.

@codecov
Copy link

codecov bot commented Feb 19, 2023

Codecov Report

Base: 69.46% // Head: 69.59% // Increases project coverage by +0.13% 🎉

Coverage data is based on head (e8932a8) compared to base (223c67f).
Patch coverage: 80.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #487      +/-   ##
==========================================
+ Coverage   69.46%   69.59%   +0.13%     
==========================================
  Files          90       90              
  Lines        5737     5710      -27     
==========================================
- Hits         3985     3974      -11     
+ Misses       1571     1560      -11     
+ Partials      181      176       -5     
Impacted Files Coverage Δ
dhcpv6/dhcpv6message.go 55.35% <41.37%> (-1.25%) ⬇️
dhcpv6/dhcpv6relay.go 69.51% <50.00%> (-2.72%) ⬇️
dhcpv6/option_ntp_server.go 66.66% <69.23%> (+3.25%) ⬆️
dhcpv6/option_fqdn.go 83.33% <80.00%> (-0.88%) ⬇️
dhcpv6/option_iaprefix.go 81.81% <87.50%> (-1.24%) ⬇️
dhcpv6/option_iapd.go 66.66% <88.23%> (+1.96%) ⬆️
dhcpv6/option_nontemporaryaddress.go 77.94% <88.23%> (-0.03%) ⬇️
dhcpv6/option_4rd.go 85.56% <100.00%> (+0.95%) ⬆️
dhcpv6/option_archtype.go 100.00% <100.00%> (ø)
dhcpv6/option_bootfileparam.go 86.36% <100.00%> (+9.44%) ⬆️
... and 24 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hugelgupf hugelgupf force-pushed the restrict branch 2 times, most recently from e8a6e5b to ee037bd Compare February 19, 2023 08:02
@hugelgupf hugelgupf closed this Feb 19, 2023
@hugelgupf hugelgupf reopened this Feb 19, 2023
@hugelgupf hugelgupf marked this pull request as draft February 19, 2023 19:41
Signed-off-by: Chris Koch <[email protected]>
…tions

RFC 8415 Appendix B and C describe which option codes are allowed in
which options, for most of them anyway.

Signed-off-by: Chris Koch <[email protected]>
Tests that for the correct option code, the correct deserialization is
applied.

Signed-off-by: Chris Koch <[email protected]>
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.

1 participant