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

Change qr bit #203

Open
CrabNejonas opened this issue Apr 26, 2023 · 3 comments
Open

Change qr bit #203

CrabNejonas opened this issue Apr 26, 2023 · 3 comments

Comments

@CrabNejonas
Copy link
Contributor

The DNS packet header bit QR is set to 0 for queries and 1 for a response.
domain::base::header::Header casts this to a bool where 0 becomes false and 1 becomes true.

This is incredibly confusing to me since qr is clearly meant to mean query, so when you use the qr() method you basically have to to the opposite of what the header field indicates.

While I agree this is technically correct (as it just describes wether the flag is set) this took me about 1hr and largely re-implementing a dns parser from scratch to figure out. Soooo maybe we should do something about that 😄

@partim
Copy link
Member

partim commented Apr 28, 2023

I agree that the definition of the QR bit is unfortunate. However, having a method that returns false when the bit is set only makes things worse. What I think we could do is adding a separate method is_query that does the flipping. The more elaborate name would likely cause people to pick that one over plain qr, so I think that is a good solution?

@CrabNejonas
Copy link
Contributor Author

Yeah that could be a good idea, another idea I had was changing qr() to return an enum instead of a bool, so

#[repr(u8)]
enum QR {
   Query = 0,
   Response = 1
}

that way it's impossible to get confused

@partim
Copy link
Member

partim commented May 1, 2023

Hm, yes. But then this flag behaves differently from the other flags, which I kind of dislike. Also, msg.is_query() is shorter and perhaps more straightforward than matches!(msg.qr(), Qr::Query).

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

No branches or pull requests

2 participants