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

Small improvements for dig-compatible output. #38

Merged
merged 3 commits into from
Mar 25, 2025

Conversation

Philip-NLnetLabs
Copy link
Member

I started with 'do' instead of 'true'. But I also improved the question section and the value of the TCP Keepalive option.

Copy link
Contributor

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Two minor suggestions, but otherwise good to merge. I can fix these if you want.

Comment on lines 68 to 71
opt.timeout().map_or("".to_string(), |t| format!(
"{:.1}",
Duration::from(t).as_secs_f64()
))
Copy link
Contributor

Choose a reason for hiding this comment

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

Small suggestion here. Dig prints the TCPKEEPALIVE without "secs" if the value is missing. It also has a space between the two words.

; <<>> DiG 9.18.33 <<>> google.com A +tcp +keepalive +qr
;; global options: +cmd
;; Sending:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 32748
;; flags: rd ad; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1232
; COOKIE: 916ce5e901a627c4
; TCP KEEPALIVE:
;; QUESTION SECTION:
;google.com.                    IN      A
...

@@ -46,7 +47,7 @@ pub fn write(
target,
"; EDNS: version {}; flags: {}; udp: {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that dig does a , instead of a ; after the version

@tertsdiepraam
Copy link
Contributor

@Philip-NLnetLabs I can't request review because it's your PR, but it would be great if you could review these fixes.

@Philip-NLnetLabs
Copy link
Member Author

Those look good to me.

@tertsdiepraam tertsdiepraam merged commit 269c37c into main Mar 25, 2025
24 checks passed
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.

2 participants