-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
Introduce a new base
module
#474
base: main
Are you sure you want to change the base?
Conversation
It implements 'Hash' for the provided integer types.
fn parse_from_message( | ||
message: &'a Message, | ||
start: usize, | ||
) -> Result<Self, ParseError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not call split_from_message and check that rest is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While that would be a valid implementation, I am hoping to define new proc macros for SplitFromMessage
and ParseFromMessage
soon (in the same fashion as SplitBytes
and ParseBytes
). I chose to define this method with a bit more boilerplate so that I can refer to it as a template when writing the proc macro.
#[derive(Clone, BuildBytes, ParseBytes, SplitBytes)] | ||
pub struct Question<N> { | ||
/// The domain name being requested. | ||
pub qname: N, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for names there needs to be some underlying trait that specifies that N can parse and build compressed DNS names. Otherwise, for example, compressed and uncompressed names can get confused or other confusion may arise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is strongly expected that any domain name type (at least those in this crate) will implement ParseFromMessage
, SplitFromMessage
and BuildIntoMessage
using name (de)compression. It is currently possible to use an incorrect type here, but I believe the risks of such a confusion are quite low. In the future, if we introduce new name types which do not follow these conventions, we can introduce e.g. a Name
trait.
src/new_edns/cookie.rs
Outdated
#[cfg(all(feature = "std", feature = "siphasher"))] | ||
use crate::new_base::wire::{AsBytes, TruncationError}; | ||
|
||
//----------- CookieRequest -------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this very confusing. There are two types of cookies, short cookies that only contain the client's cookie and longer cookies that contain both the client and the server parts. There are two roles, the client and the server.
It is unclear to me what is what and who does what.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A CookieRequest
is a "client coookie", which requests a Cookie
("server cookie") from the server. I tried these names out as I found them more intuitive than ClientCookie
and ServerCookie
, but I'm happy to switch to the conventional names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clients send two types of cookies, the small one and the big. And receive only the big one. Servers receive small and big and send only big.
Another issue might be that if a client receives a server cookie, we cannot assume anything about what the server put there. The server might just send some random bytes.
Also fixes typo in field name 'reversed' ('reserved') of 'Cookie'. See: <#474 (comment)> See: <#474 (comment)>
- The traits have been renamed to '{Parse,Split}MessageBytes' for consistency with '{Parse,Split}Bytes'. - The traits now consume 'message.contents' instead of the whole 'message', allowing them to be used in more contexts (e.g. the upcoming overhauled builder types).
- 'BuilderContext' now tracks the last question / record in a message, allowing its building to be recovered in the future (WIP). - 'MessageBuilder' inlines the 'Builder' and just stores a mutable reference to a 'Message' for simplicity. - 'Builder' no longer gives access to the message header, and uses '&UnsafeCell<[u8]>' to represent the message contents.
I have taken care to preserve explicit parse impls for wrapper types like 'SizePrefixed', and a few cases where performance could matter.
This introduces a
new_base
module as a potential candidate for replacingbase
. While it has a similar structure, it has some distinguishing features:It uses concrete types (usually slices and references) in place of
Octs
generic parameters. The hard-coded types are ergonomic and efficient and simplify the API.It introduces a new parsing architecture which aggressively avoids copying. The architecture is inspired by the
zerocopy
crate, but allows for partial copying when zero-copy is not possible.derive
macros are provided (and heavily used internally) to avoid boilerplate when implementing the parsing traits.It overhauls the message building process so that truncation errors are handled more gracefully and message components (i.e. questions and records) can be built incrementally. This lays the groundwork for a more efficient client/server architecture where messages are built in-place, even across middleware layers.
It introduces
RevName
, a more efficient type for domain names (where labels are stored in reverse order). Operations onRevName
tend to be more efficient (e.g. canonical-order comparisons are significantly better).Here's the current roadmap:
derive
macros for the parsing/building traits.derive
macros for these traits.derive
macros for these traits.net::server
(in a separate PR) on top ofnew-base
.Note: this is a new version of #469 with a less problematic branch name.