Add ERCodeRule #6147
Merged
Add ERCodeRule #6147
Conversation
|
Nice work, just a few minor nits! |
| bool matches(const DNSQuestion* dq) const override | ||
| { | ||
| // avoid parsing EDNS OPT RR when not needed. | ||
| if (d_rcode != dq->dh->rcode) { |
rgacogne
Jan 8, 2018
Member
Given that dq->dh->rcode is unsigned, perhaps d_rcode should be too? Same for d_extrcode since edns0.extRCode is an uint8_t.
Given that dq->dh->rcode is unsigned, perhaps d_rcode should be too? Same for d_extrcode since edns0.extRCode is an uint8_t.
pdns/dnsdist-lua-rules.cc
Outdated
| char * optStart = NULL; | ||
| size_t optLen = 0; | ||
| bool last = false; | ||
| int res = locateEDNSOptRR((char*)dq->dh, dq->len, &optStart, &optLen, &last); |
rgacogne
Jan 8, 2018
Member
const_cast<char*>(reinterpret_cast<const char*>(dq->dh)) ? I don't think we'll manage to get rid of the existing C-style casts, but it would be nice to add new ones as few as possible :)
const_cast<char*>(reinterpret_cast<const char*>(dq->dh)) ? I don't think we'll manage to get rid of the existing C-style casts, but it would be nice to add new ones as few as possible :)
pdns/dnsdist-lua-rules.cc
Outdated
| } | ||
| EDNS0Record edns0; | ||
| static_assert(sizeof(EDNS0Record) == sizeof(uint32_t), "sizeof(EDNS0Record) must match sizeof(uint32_t) AKA RR TTL size"); | ||
| memcpy(&edns0, optStart + 5, sizeof edns0); |
rgacogne
Jan 8, 2018
Member
Adding a comment explaining where does the 5 comes from might be nice for later review.
Adding a comment explaining where does the 5 comes from might be nice for later review.
|
Nits addressed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Short description
Add ERCodeRule to match on EDNS Extended RCodes.
Separate rule to avoid performance cost for non-EDNS packets. Unclear if this is a good idea for correctness.
Checklist
I have: