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

Move Algorithm::from_str()'s logic from -proto to -server. #259

Merged
merged 3 commits into from Oct 25, 2017
Merged

Move Algorithm::from_str()'s logic from -proto to -server. #259

merged 3 commits into from Oct 25, 2017

Conversation

briansmith
Copy link
Contributor

Algorithm::from_str() isn't uesd by -resolver so it shouldn't be in
-proto.

briansmith and others added 2 commits October 24, 2017 14:08
`Algorithm::from_str()` isn't uesd by -resolver so it shouldn't be in
-proto.
@bluejekyll bluejekyll self-requested a review October 25, 2017 02:12
@bluejekyll
Copy link
Member

I understand your reasoning here, but I need consider this. It's possible that this might want to exist in the Client in the long run... Technically it's only TRust-DNS that uses this form, as I believe the text form of the Algorithm in Records is the numerical identifier.

I get it, but I generally like keeping the logic like this close to it's type... but given it's only used in the server...

@briansmith
Copy link
Contributor Author

I understand your reasoning here, but I need consider this. It's possible that this might want to exist in the Client in the long run

I agree it could very well live in -client. I only put it in -server because there's only one use of it. I suggest we do that when something besides -server needs it. But, I'd be happy to move it to -client now, especially if the other from_str() logic is there or going to be there soon.

I get it, but I generally like keeping the logic like this close to it's type

I agree, but in general that strategy is counterproductive to the goal of minimizing the size of the TCB of the resolver and maximizing the effectiveness of the testing and auditing of -proto and -resolver. (Also in the case of -server, I could see us eventually factoring out the code specifically related to processing untrusted inputs into a separate minimized module or crate, to minimize its TCB, for the same reasons.)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 87.401% when pulling aec6258 on briansmith:remove-Algorithm-from_str into 0bb0b2c on bluejekyll:master.

@bluejekyll bluejekyll merged commit 6bf9ec2 into hickory-dns:master Oct 25, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 87.401% when pulling 2738233 on briansmith:remove-Algorithm-from_str into dcb4971 on bluejekyll:master.

@briansmith briansmith deleted the remove-Algorithm-from_str branch October 30, 2017 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants