starred/hickory-dns

Fork 0

mirror of https://github.com/hickory-dns/hickory-dns.git synced 2026-04-25 11:15:54 +03:00

[GH-ISSUE #1612] `BinEncoder` should work with anything `AsMut<[u8]>` #712

New issue

Open

opened 2026-03-15 23:55:25 +03:00 by kerem · 11 comments

kerem commented

2026-03-15 23:55:25 +03:00

Owner

Originally created by @zhuhaow on GitHub (Jan 5, 2022).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/1612

I want to dump the binary representation of the result of Message into a buffer of raw IP packet.

Working with current API requires us to use BinEncoder with pub fn with_offset(buf: &'a mut Vec<u8>, offset: u32, mode: EncodeMode) which would do the work but requires us to know the offset and own a ref to Vec which may not be the case (e.g., when we are using bytes).

But if BinEncoder can be generalized to take T: AsMut<[u8]> everything will be significantly simpler, including the API of BinEncoder.

I've checked the code and I feel it's possible to do it. The only thing is the into_bytes(self) method would be a little misleading as it's better called into_inner.

Is there anything I'm missing that prevents us doing it?

I would love to make the PR.

Originally created by @zhuhaow on GitHub (Jan 5, 2022). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/1612 I want to dump the binary representation of the result of `Message` into a buffer of raw IP packet. Working with current API requires us to use `BinEncoder` with `pub fn with_offset(buf: &'a mut Vec<u8>, offset: u32, mode: EncodeMode)` which would do the work but requires us to know the offset and own a ref to `Vec` which may not be the case (e.g., when we are using `bytes`). But if `BinEncoder` can be generalized to take `T: AsMut<[u8]>` everything will be significantly simpler, including the API of `BinEncoder`. I've checked the code and I feel it's possible to do it. The only thing is the `into_bytes(self)` method would be a little misleading as it's better called `into_inner`. Is there anything I'm missing that prevents us doing it? I would love to make the PR.

kerem commented

2026-03-15 23:55:31 +03:00

Author

Owner

@zhuhaow commented on GitHub (Jan 5, 2022):

One potential breaking change is that currently we are reserving space of the buffer for the caller, which is not possible with AsMut.

But error out when emitting is probably more reasonable in this case if there is no enough space. The caller should allocate proper space.

@zhuhaow commented on GitHub (Jan 5, 2022): One potential breaking change is that currently we are reserving space of the buffer for the caller, which is not possible with `AsMut`. But error out when emitting is probably more reasonable in this case if there is no enough space. The caller should allocate proper space.

kerem commented

2026-03-15 23:55:36 +03:00

Author

Owner

@djc commented on GitHub (Jan 5, 2022):

It seems reasonable to add some API that just takes &mut [u8] (and returns an error if there's not enough space). I don't think we'd want to get rid of the Vec<u8>-based API though.

@djc commented on GitHub (Jan 5, 2022): It seems reasonable to add some API that just takes `&mut [u8]` (and returns an error if there's not enough space). I don't think we'd want to get rid of the `Vec<u8>`-based API though.

kerem commented

2026-03-15 23:55:41 +03:00

Author

Owner

@bluejekyll commented on GitHub (Jan 6, 2022):

I guess I don't have a problem with this, but I do have some concerns with essentially asking the caller to predict the space required. I'm wondering if we could do something with specialization on Extend<u8>. It might mean a larger change though to extend with iterators (might be feasible). The compiler might get angry with two conflicting impls on T: AsMut<[u8]> + Extend<u8> and just T: AsMut<[u8]>.

Actually, could we just have a specialization on &'_ mut Vec<u8> and T: AsMut<u8>?

@bluejekyll commented on GitHub (Jan 6, 2022): I guess I don't have a problem with this, but I do have some concerns with essentially asking the caller to predict the space required. I'm wondering if we could do something with specialization on [`Extend<u8>`](https://doc.rust-lang.org/std/iter/trait.Extend.html). It might mean a larger change though to extend with iterators (might be feasible). The compiler might get angry with two conflicting impls on `T: AsMut<[u8]> + Extend<u8>` and just `T: AsMut<[u8]>`. Actually, could we just have a specialization on `&'_ mut Vec<u8>` and `T: AsMut<u8>`?

kerem commented

2026-03-15 23:55:46 +03:00

Author

Owner

@djc commented on GitHub (Jan 6, 2022):

Maybe just generalizing from &mut Vec<u8> to Extend<u8> is actually enough, if the actual goal is just to pass in a BytesMut?

@djc commented on GitHub (Jan 6, 2022): Maybe just generalizing from `&mut Vec<u8>` to `Extend<u8>` is actually enough, if the actual goal is just to pass in a `BytesMut`?

kerem commented

2026-03-15 23:55:51 +03:00

Author

Owner

@zhuhaow commented on GitHub (Jan 7, 2022):

Maybe just generalizing from &mut Vec<u8> to Extend<u8> is actually enough, if the actual goal is just to pass in a BytesMut?

In this case we may need to use AsMut<[u8]> + Extend<u8> since we won't have length information with only Extend.

Honestly I still find it a little surprising that the method will adjust the buffer size implicitly for us (both enlarge and shrink). Usually in this case when the buffer length will be changed, trait like std::io::ReadBufor bytes::BufMut is used instead of specifing the real storage type directly so the behavior is implied. Of course, generally these traits expect we work with data with either known size or chuck of a stream with arbitrary size, which is not applicable in this case.

@zhuhaow commented on GitHub (Jan 7, 2022): > Maybe just generalizing from `&mut Vec<u8>` to `Extend<u8>` is actually enough, if the actual goal is just to pass in a `BytesMut`? In this case we may need to use `AsMut<[u8]> + Extend<u8>` since we won't have length information with only `Extend`. Honestly I still find it a little surprising that the method will adjust the buffer size implicitly for us (both enlarge and shrink). Usually in this case when the buffer length will be changed, trait like `std::io::ReadBuf`or `bytes::BufMut` is used instead of specifing the real storage type directly so the behavior is implied. Of course, generally these traits expect we work with data with either known size or chuck of a stream with arbitrary size, which is not applicable in this case.

kerem commented

2026-03-15 23:55:57 +03:00

Author

Owner

@djc commented on GitHub (Jan 7, 2022):

ReadBuf is still unstable. BufMut seems like a reasonable option to me.

@djc commented on GitHub (Jan 7, 2022): ReadBuf is still unstable. BufMut seems like a reasonable option to me.

kerem commented

2026-03-15 23:56:02 +03:00

Author

Owner

@zhuhaow commented on GitHub (Jan 7, 2022):

However, we cannot resize with BufMut, which then requires us to ask the caller to provide a buffer that is large enough.

@zhuhaow commented on GitHub (Jan 7, 2022): However, we cannot resize with `BufMut`, which then requires us to ask the caller to provide a buffer that is large enough.

kerem commented

2026-03-15 23:56:07 +03:00

Author

Owner

@zhuhaow commented on GitHub (Jan 7, 2022):

I still think it makes sence to enable user to dump the result to a buffer with enough size and errors out when it doesn't.

This avoids a potential copy most of the time.

@zhuhaow commented on GitHub (Jan 7, 2022): I still think it makes sence to enable user to dump the result to a buffer with enough size and errors out when it doesn't. This avoids a potential copy most of the time.

kerem commented

2026-03-15 23:56:12 +03:00

Author

Owner

@djc commented on GitHub (Jan 7, 2022):

That might be reasonable for your use case, but other users might not care enough about the performance implications of the reallocation and prefer the ergonomics of something that's infallible.

@djc commented on GitHub (Jan 7, 2022): That might be reasonable for your use case, but other users might not care enough about the performance implications of the reallocation and prefer the ergonomics of something that's infallible.

kerem commented

2026-03-15 23:56:17 +03:00

Author

Owner

@bluejekyll commented on GitHub (Jan 7, 2022):

I suppose it's possible for us to change the API to generally be used with a predefined buffer size. We use 2k as the limit for EDNS today. The real negative with making this fallible and requiring the caller to reallocate is that I think that would mean reencoding the entire Message, which would incur a performance penanlty. The only issue with that is that I know we have some Messages in the server that are encoded once and use some iterators for the records. That would need to be updated in order to make reencoding possible.

@bluejekyll commented on GitHub (Jan 7, 2022): I suppose it's possible for us to change the API to generally be used with a predefined buffer size. We use 2k as the limit for EDNS today. The real negative with making this fallible and requiring the caller to reallocate is that I think that would mean reencoding the entire Message, which would incur a performance penanlty. The only issue with that is that I know we have some Messages in the server that are encoded once and use some iterators for the records. That would need to be updated in order to make reencoding possible.

kerem commented

2026-03-15 23:56:22 +03:00

Author

Owner

@daxpedda commented on GitHub (Oct 4, 2023):

In #1987 this issue has come up as well. The type we receive from h3 is a impl Buf, so the requirement here is either taking impl Buf or impl Read.

@daxpedda commented on GitHub (Oct 4, 2023): In #1987 this issue has come up as well. The type we receive from `h3` is a [`impl Buf`](https://docs.rs/bytes/1.5.0/bytes/buf/trait.Buf.html), so the requirement here is either taking `impl Buf` or `impl Read`.