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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass DnsRequestOptions to DNSSEC validating routines #1742

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

wiktor-k
Copy link
Contributor

Hello,

This PR fixes the issue reported in #1735 and makes sure that the DnsRequestOptions that the client specifies are being forwarded to the DNSSEC validating functions.

The change is basically grabbing a copy of the options (because request is passed by value to send the original options cannot be used) and then adding the options parameter to validate_* functions and using that instead of DnsRequestOptions::default(). The additional parameter should not affect public API since all these functions are not marked pub.

When this PR gets merged I'll follow up with the "configurable max_request_depth" PR that doesn't make sense if the options are not being forwarded in the first place :).

Suggestions welcome! 馃憢

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

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

This mostly looks good to me. It looks like a bunch of functions are taking a &DnsRequestOptions only to dereference it when using. If we're just going to dereference it anyway, why not take ownership? The mix between passing references and owned values seems confusing.

@wiktor-k
Copy link
Contributor Author

Good idea, I'll fix that and re-submit. Thanks!

@wiktor-k wiktor-k force-pushed the fix-dns-req-opts branch 2 times, most recently from 230d3f0 to 4b4e520 Compare July 25, 2022 10:32
@wiktor-k
Copy link
Contributor Author

I've adjusted the code. With your suggestions applied it looks cleaner. I couldn't avoid all explicit dereferences since options() returns a ref in DnsRequest but I don't think this is a big issue.

@wiktor-k
Copy link
Contributor Author

wiktor-k commented Aug 1, 2022

Sorry for the noise @bluejekyll but what's the workflow for getting an approved PRs merged in here? I'm wondering if I can be of help in here...

@djc
Copy link
Collaborator

djc commented Aug 1, 2022

Hey, sorry for the slow feedback cycle here. I often like to get an additional review from @bluejekyll before a PR is merged, but he is currently on vacation. In the absence of his review, this is straightforward enough that I'll just merge it now.

@djc djc merged commit 64f35b6 into hickory-dns:main Aug 1, 2022
@wiktor-k
Copy link
Contributor Author

wiktor-k commented Aug 1, 2022

Ah, no worries. I asked because I've got yet another PR I planned to submit that depends on this one...

Thanks for the quick response and have a nice day! 馃憢

@wiktor-k wiktor-k deleted the fix-dns-req-opts branch August 1, 2022 08:44
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.

None yet

2 participants