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.

Sign up for GitHub

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

add OctetsFrom implementations for common octet types #126

Closed
wants to merge 1 commit into from
Closed

add OctetsFrom implementations for common octet types #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/base/name/dname.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use super::super::octets::{
use super::super::octets::{DeserializeOctets, SerializeOctets};
use super::builder::{DnameBuilder, FromStrError};
use super::label::{Label, LabelTypeError, SplitLabelError};
use super::parsed::ParsedDname;
use super::relative::{DnameIter, RelativeDname};
use super::traits::{ToDname, ToLabelIter};
#[cfg(feature = "master")]
Expand Down Expand Up @@ -624,6 +625,17 @@ where
}
}

impl<SrcOctets, Octets> OctetsFrom<ParsedDname<SrcOctets>> for Dname<Octets>
where
SrcOctets: AsRef<[u8]>,
Octets: FromBuilder,
<Octets as FromBuilder>::Builder: EmptyBuilder,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually a blanket implementation from the previous PR. But it also converts ParsedDname -> Dname. I think it can either work like this, or or something like to_dname::<SrcOctets>()?.octets_into().

Copy link
Member

Choose a reason for hiding this comment

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

Isn’t just to_dname enough here? If this is about the error type – not sure why that is PushError in ToDname::to_dname. The type implementing ToDname should always have a valid domain name, so the only error is a ShortBuf. Unless I am missing something, I happily change the type in the trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is so you can convert Record<ParsedDname<Src>, Src> to Record<Dname<Src>, Src> using octets conversion instead of rebuilding the record (which is useful when storing a record from message for example), but let me try to rebase without these changes and see if we need it.

Copy link
Member

Choose a reason for hiding this comment

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

Feels like there should be a better way to do this: if the name isn’t compressed, this can be done very cheaply for certain types. Maybe have a method flatten on ParseDname<_> and Record<ParseDname<_>, _>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that too! So how is flatten different from ToDname? It should be possible to optimize ToDname the same way (conversion is cheaper when as_flat_slice is some or octets can be shallow copied/referenced).

Should flatten be something like Dname::as_octets but return a an Option? (so for example ParsedDname<&Bytes> should return Option<&Bytes> which could be cheap to convert into Dname or Dname<&Bytes> as ParsedDname guarantees that the octets are a valid Dname). Is that what you mean? Or flatten would convert both type (ParsedDname -> Dname) and octets type (like this method)?

For me this is just a shortcut for building records from &[u8] and I rarely want to shallow copy Bytes backed records, so I'm also happy to drop this particular change, as it's just a convenience.

Copy link
Member

@partim partim May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The idea would be that if the parsed name is not compressed, flatten can simply return with self.parser.parse_octets(...) while otherwise it constructs the name via Dname::to_dname. I think this should be possible with slightly extravagant trait bounds – you’d need <Ref as OctetsRef>::Range to be FromBuilder plus some more things to tie it together.

Edit: I guess that wouldn’t actually work for your case, since you still need an octets conversion. Perhaps flatten_into instead with a type argument for a target octets type? I imagine it is cheaper to just copy the octets on an uncompressed name than constructing it label by label.

Copy link
Member

Choose a reason for hiding this comment

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

How do you want to proceed? I want to merge #130 for other work I am doing but there’s some overlap, so I would rather merge this PR first to avoid things getting too messy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't been able to get back to this yet but let me decline it for now. I think I can make do without the nicer interface (or add extension traits to my library for record building with octets conversion) for now and will resubmit this if needed.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Feel free to propose or request something at any time! I might implement the two proposed flatten methods anyway – that seems like a useful thing to have.

{
fn octets_from(source: ParsedDname<SrcOctets>) -> Result<Self, ShortBuf> {
source.to_dname().map_err(|_| ShortBuf)
}
}

//--- FromStr

impl<Octets> FromStr for Dname<Octets>
Expand Down
192 changes: 169 additions & 23 deletions src/base/octets.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,32 +371,16 @@ impl<'a, Source: AsRef<[u8]> + 'a> OctetsFrom<&'a Source> for &'a [u8] {
}

#[cfg(feature = "std")]
impl<Source> OctetsFrom<Source> for Vec<u8>
where
Self: From<Source>,
{
fn octets_from(source: Source) -> Result<Self, ShortBuf> {
Ok(From::from(source))
impl<'a> OctetsFrom<&'a [u8]> for Vec<u8> {
fn octets_from(source: &'a [u8]) -> Result<Self, ShortBuf> {
Ok(Self::from(source))
}
}

#[cfg(feature = "bytes")]
impl<Source> OctetsFrom<Source> for Bytes
where
Self: From<Source>,
{
fn octets_from(source: Source) -> Result<Self, ShortBuf> {
Ok(From::from(source))
}
}

#[cfg(feature = "bytes")]
impl<Source> OctetsFrom<Source> for BytesMut
where
Self: From<Source>,
{
fn octets_from(source: Source) -> Result<Self, ShortBuf> {
Ok(From::from(source))
#[cfg(feature = "std")]
impl OctetsFrom<Vec<u8>> for Vec<u8> {
fn octets_from(source: Vec<u8>) -> Result<Self, ShortBuf> {
Ok(source)
}
}

Expand All @@ -411,6 +395,76 @@ where
}
}

#[cfg(feature = "bytes")]
impl<'a> OctetsFrom<&'a [u8]> for Bytes {
fn octets_from(source: &'a [u8]) -> Result<Self, ShortBuf> {
Ok(Self::copy_from_slice(source))
}
}

#[cfg(feature = "bytes")]
impl OctetsFrom<Bytes> for Bytes {
fn octets_from(source: Bytes) -> Result<Self, ShortBuf> {
Ok(source)
}
}

#[cfg(feature = "bytes")]
impl OctetsFrom<BytesMut> for BytesMut {
fn octets_from(source: BytesMut) -> Result<Self, ShortBuf> {
Ok(source)
}
}

#[cfg(feature = "bytes")]
impl OctetsFrom<Bytes> for BytesMut {
fn octets_from(source: Bytes) -> Result<Self, ShortBuf> {
Ok(Self::from(source.as_ref()))
}
}

#[cfg(feature = "bytes")]
impl OctetsFrom<BytesMut> for Bytes {
fn octets_from(source: BytesMut) -> Result<Self, ShortBuf> {
Ok(Self::from(source))
}
}

#[cfg(feature = "bytes")]
impl<'a> OctetsFrom<&'a [u8]> for BytesMut {
fn octets_from(source: &'a [u8]) -> Result<Self, ShortBuf> {
Ok(Self::from(source))
}
}

#[cfg(feature = "bytes")]
impl OctetsFrom<Vec<u8>> for Bytes {
fn octets_from(source: Vec<u8>) -> Result<Self, ShortBuf> {
Ok(Self::from(source))
}
}

#[cfg(feature = "bytes")]
impl OctetsFrom<Vec<u8>> for BytesMut {
fn octets_from(source: Vec<u8>) -> Result<Self, ShortBuf> {
Ok(Self::from(source.as_slice()))
}
}

#[cfg(feature = "bytes")]
impl OctetsFrom<Bytes> for Vec<u8> {
fn octets_from(source: Bytes) -> Result<Self, ShortBuf> {
Ok(source.as_ref().to_vec())
}
}

#[cfg(feature = "bytes")]
impl OctetsFrom<BytesMut> for Vec<u8> {
fn octets_from(source: BytesMut) -> Result<Self, ShortBuf> {
Ok(source.as_ref().to_vec())
}
}

#[cfg(feature = "heapless")]
impl<Source, const N: usize> OctetsFrom<Source> for heapless::Vec<u8, N>
where
Expand Down Expand Up @@ -1884,6 +1938,41 @@ macro_rules! octets_array {
}
}

impl OctetsFrom<$name> for $name {
fn octets_from(source: $name) -> Result<Self, ShortBuf> {
Ok(source)
}
}

#[cfg(feature = "std")]
impl OctetsFrom<Vec<u8>> for $name {
fn octets_from(source: Vec<u8>) -> Result<Self, ShortBuf> {
let octets = <[u8; $len]>::try_from(source).map_err(|_| ShortBuf)?;
Ok($name { octets, len: $len })
}
}

#[cfg(feature = "std")]
impl OctetsFrom<$name> for Vec<u8> {
fn octets_from(source: $name) -> Result<Self, ShortBuf> {
Ok(source.octets.into())
}
}

#[cfg(feature = "bytes")]
impl OctetsFrom<$name> for Bytes {
fn octets_from(source: $name) -> Result<Self, ShortBuf> {
Ok(source.octets.to_vec().into())
}
}

#[cfg(feature = "bytes")]
impl OctetsFrom<$name> for BytesMut {
fn octets_from(source: $name) -> Result<Self, ShortBuf> {
Ok(BytesMut::from(source.octets.as_ref()))
}
}

impl core::ops::Deref for $name {
type Target = [u8];

Expand Down Expand Up @@ -2260,4 +2349,61 @@ mod test {
assert_eq!(parser.parse_u32(), Ok(0xfd78a84e));
assert_eq!(parser.parse_u32(), Err(ParseError::ShortInput));
}

#[cfg(feature = "std")]
#[test]
fn convert() {
use crate::base::octets::{Octets32, OctetsInto};
use crate::base::{Dname, Record};
use crate::rdata::Cname;

// Use case: provide a flexible interface for caller to
// get record from storage.
fn load_record<O>() -> Record<Dname<O>, Cname<Dname<O>>>
where
O: FromBuilder + OctetsFrom<Octets32>,
<O as FromBuilder>::Builder: EmptyBuilder,
{
// let rr = cache_get().clone();
let rr: Record<Dname<Octets32>, Cname<Dname<Octets32>>> = (
"a.example".parse().unwrap(),
300,
Cname::new("b.example".parse().unwrap()),
)
.into();

rr.octets_into().unwrap()
}
let rr: Record<Dname<Vec<u8>>, Cname<Dname<Vec<u8>>>> = load_record();

let rr_ref: Record<_, _> = (
rr.owner().for_slice(),
rr.ttl(),
Cname::new(rr.data().cname().for_slice()),
)
.into();

// Use case: save a owned copy into cache.
fn save_record<O>(rr: Record<Dname<O>, Cname<Dname<O>>>)
where
O: AsRef<[u8]>,
Vec<u8>: OctetsFrom<O>,
{
let _owned: Record<Dname<Vec<u8>>, Cname<Dname<Vec<u8>>>> =
rr.octets_into().unwrap();
// cache_put(_owned)
}
save_record(rr_ref.clone());

// convert between octets
#[cfg(feature = "bytes")]
{
use bytes::Bytes;

let b: Record<Dname<Bytes>, Cname<Dname<Bytes>>> =
rr_ref.clone().octets_into().unwrap();
let _: Record<Dname<Vec<u8>>, Cname<Dname<Vec<u8>>>> =
b.octets_into().unwrap();
}
}
}
18 changes: 10 additions & 8 deletions src/base/record.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,22 +1007,24 @@ impl<N, D> From<ShortBuf> for RecordParseError<N, D> {
mod test {

#[test]
#[cfg(features = "bytes")]
#[cfg(feature = "bytes")]
fn ds_octets_into() {
use crate::base::iana::{DigestAlg, Rtype, SecAlg};
use crate::name::Dname;
use crate::octets::OctetsInto;
use crate::base::iana::{Class, DigestAlg, SecAlg};
use crate::base::octets::OctetsInto;
use crate::base::{Dname, Record};
use crate::rdata::Ds;
use bytes::Bytes;

let owner = "a.example".parse::<Dname<Bytes>>().unwrap();
let ds: Record<Dname<&[u8]>, Ds<&[u8]>> = Record::new(
"a.example".parse().unwrap(),
owner.for_slice(),
Class::In,
86400,
Ds::new(12, SecAlg::RsaSha256, b"something"),
Ds::new(12, SecAlg::RsaSha256, DigestAlg::Sha256, b"something"),
);
let ds_bytes: Record<Dname<Bytes>, Ds<Bytes>> =
ds.octets_into().unwrap();
ds.clone().octets_into().unwrap();
assert_eq!(ds.owner(), ds_bytes.owner());
asswer_eq!(ds.data().digest(), ds_bytes.data().digest());
assert_eq!(ds.data().digest(), ds_bytes.data().digest());
}
}
13 changes: 7 additions & 6 deletions src/rdata/rfc1035.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1841,29 +1841,30 @@ mod test {
use std::vec::Vec;

#[test]
#[cfg(features = "bytes")]
#[cfg(feature = "bytes")]
fn hinfo_octets_into() {
use crate::octets::OctetsInto;
use crate::base::octets::OctetsInto;

let hinfo: Hinfo<Vec<u8>> =
Hinfo::new("1234".parse().unwrap(), "abcd".parse().unwrap());
let hinfo_bytes: Hinfo<bytes::Bytes> = hinfo.octets_into().unwrap();
let hinfo_bytes: Hinfo<bytes::Bytes> =
hinfo.clone().octets_into().unwrap();
assert_eq!(hinfo.cpu(), hinfo_bytes.cpu());
assert_eq!(hinfo.os(), hinfo_bytes.os());
}

#[test]
#[cfg(features = "bytes")]
#[cfg(feature = "bytes")]
fn minfo_octets_into() {
use crate::base::octets::OctetsInto;
use crate::base::Dname;
use crate::octets::OctetsInto;

let minfo: Minfo<Dname<Vec<u8>>> = Minfo::new(
"a.example".parse().unwrap(),
"b.example".parse().unwrap(),
);
let minfo_bytes: Minfo<Dname<bytes::Bytes>> =
minfo.octets_into().unwrap();
minfo.clone().octets_into().unwrap();
assert_eq!(minfo.rmailbx(), minfo_bytes.rmailbx());
assert_eq!(minfo.emailbx(), minfo_bytes.emailbx());
}
Expand Down