Introduce OctetsFrom and OctetsInto.

[partim]

This continues from PR #69.

I think I have implemented OctetsFrom for all relevant types with OctetsInto automatically available for them via a blanket impl. @vavrusa, can you have a look and let me know if this works for you or what needs changing?

[vavrusa]

Yeah! Can you add the tests from PR#69? It should make it more obvious whether it works or not. The implementation for Record is missing. I'm trying to figure out how the traits bound would look like now. One example - converting record with a DS into another record with generic octets.

Previously it would look something like this:

fn convert_ds<O>() -> Record<Dname<O>, Ds<O>> where
    O: FromBuilder,
    <O as FromBuilder>::Builder: EmptyBuilder {
    let ds: Record<Dname<bytes::Bytes>, Ds<bytes::Bytes>> = Record::new(...);
    ds.convert().unwrap()
}

fn test_convert() {
    let t1 = convert_ds::<Vec<u8>>();
    let t2 = convert_ds::<bytes::Bytes>();
}

Now I have:

fn convert_ds<'a, O>() -> Record<Dname<O>, Ds<O>> where
    O: FromBuilder + OctetsFrom<&'a [u8]>,
    <O as FromBuilder>::Builder: EmptyBuilder {

    let ds: Record<Dname<bytes::Bytes>, Ds<bytes::Bytes>> = Record::new(..);

    // This doesn't compile, it would need something like for_slice() to be able to convert Ds<Bytes> -> Ds<&[u8]> -> Ds<O>
    let rdata_slice: Ds<&[u8]> = ds.data().octets_into().unwrap();
    Record::new(ds.owner().for_slice().octets_into().unwrap(), Class::In, 0, rdata_slice.octets_into().unwrap())
}

fn test_convert() {
    let t1 = convert_ds::<Vec<u8>>();
    let t2 = convert_ds::<bytes::Bytes>();
}

[partim]

I have added tests to rfc1035. I think that’s the ones you had?

An impl for Record is present. I also added a test for a Record<_, Ds<_>> that seems to simply work. You don’t even need a separate function anymore, just call octets_into on your source record.

If you do want to use a function, I think just using OctetsInto or OctetsFrom (depending on your direction) should be enough.

[partim]

One thing that I didn’t add is an impl on ParsedDname. The reason is that that type is atop a OctetsRef, not an octets sequence itself. Converting that would require to convert the octets sequence used by the parser underlying the parsed name, but then you likely end up with lifetime issues because ParsedDname may hold a reference to that underlying octets sequence.

But maybe this just works out and I worry too much?

[partim]

@vavrusa, does this work for you now or is there still some usability issues to flesh out?

[vavrusa]

Hi @partim sorry about the delay, I was distracted with other things. I'll make a note to try it out again this week. If I remember correctly I added Convert implementation on ParsedDname that converted into Dname. The reason for that was so that I could convert Record<N, D> from Message into something I can store in cache. It's possible without it, just that I can't do rr.into() but have to recreate the record and convert owner using ToDname, and data using Convert.

[vavrusa]

This blanket implementation is not enough. Bytes for example only supports conversion from &'static [u8]. So converting from &Record now requires cloning the record and converting each member using the owned value. Perhaps the blanket implementation could use OctetsBuilder which can convert any Source: AsRef<[u8]> into Octets?

[partim]

But isn’t that rather a missing impl OctetsFrom<&'_ Record<...>> for Record<...>? Or am I misunderstanding you?

[vavrusa]

Ah yes you're right, I misread this, I thought it was implemented for any AsRef<[u8]>. This is not unique to Record though, any type composed of Octets that is convertible currently moves the original variable. I guess it's not going to be a big problem with Bytes as it can efficiently convert from a cloned vec, and cloning of bytes in order to be converted back to vec is cheap so it should be ok for now. impl<O> OctetsFrom<&'a Record<O, ...>> for Record<O, ...> where O: Clone would be convenient, that would make the usability similar to how Dname like types can be converted using to_dname.

[partim]

It think when doing from a &Record<O> to a Record<OO>, we should go via &O: OctetsRef. So perhaps the trait bound should be where &O: OctetsRef, OO: OctetsFrom<&O> (plus lifetimes, of course). There might be a blanket impl in there somewhere, but it probably won’t work because of specialization rules.

Perhaps explore this in a different PR? Just so we get some progress here?

[vavrusa]

Maybe add an implementation for N: SrcName instead? SrcName should have ToDname, so converting an ParsedRecord<impl OctetsRef> into Record<Dname<O>, ...> should be possible.

[partim]

I feel that this sort of thing should happen via regular From from a ParsedDname to its Dname given that it actually copies things around. The intention of OctetsFrom was really just a type conversion for the particular octet type.

[vavrusa]

Yes that's fair, however scanning ParsedRecord and converting is common and currently very clunky.

let rr = rr.into_record::<Dnskey<_>>()?.context("empty record")?;
let rr: Record<Dname<O>, Dnskey<O>> = Record::new(rr.owner().to_dname().context("failed to convert dname")?, rr.class(), rr.ttl(), rr.into_data().octets_into().context("failed to convert data")?);

If we add impl From<ParsedDname<R>> for Dname<O> that should make Record::from_record work, another option is to move this into another method/trait.

[partim]

I think there indeed lots of From impls and other conversion helpers missing. My plan was to add them as they become necessary so as to not add too much stuff that nobody uses and that just increases the code size.

I suggest we look at conversions for records and names in a separate PR.

[vavrusa]

Yes, of course, I didn't mean to hold this up, sorry.

[partim]

Nono! I really appreciate the feedback. Here is as good as anywhere.