refactor: use `Duration` instead of `u32` #202

CrabNejonas · 2023-04-24T13:34:26Z

Use the proper Duration instead of u32 for time values such as ttl.

This leads to less error prone and more readable code since plain u32 values carry no information about unit. Using Duration ensures time values are always correctly given as seconds.

Use the proper `Duration` instead of `u32` for time values such as ttl. This leads to less error prone and more readable code since plain `u32` values carry no information about unit. Using `Duration ` ensures time values are always correctly given as seconds.

partim · 2023-04-25T10:14:31Z

Thank you for the PR!

I agree that we should use a dedicated type for TTL values rather than just the raw u32. However, I think because Duration provides sub-second precision it is the wrong choice here. I think it would be better to have a newtype Ttl (it could live in base::record) around the u32 that provides explicit conversions to and from Duration.

Would you be interested to adjust your PR accordingly – otherwise I can also do the changes.

CrabNejonas · 2023-04-25T11:29:35Z

Good point, using Duration isn't really applicable here and it also doesn't expose information commonly used when working with durations in DNS (e.g. ttl in minutes or hours). I added a custom Ttl type that wraps a u32 and mirrors the methods exposed by Duration.

Only issue I have with the change is the naming, Ttl feels like a weird name for this. I was thinking maybe LowPrecisionDuration or LongDuration would be more appropriate? But also way more verbose ofc

partim · 2023-04-25T11:42:34Z

I agree Ttl isn’t perfect, but then again it is super short which is nice, and basically everyone working with DNS knows what it implies. So I think we should just go with it.

src/base/record.rs

src/base/wire.rs

src/rdata/rfc1035.rs

partim · 2023-04-25T12:05:55Z

There’s a weird quirk in RFC 1034 (or 1035) where is mistakenly declared TTL at one point to be a signed number. RFC 2181 later made it officially unsigned but said that any value with the most significant bit said should be treated as zero. Years later, RFC 8767 suggested that that was a bad idea and made the TTL properly u32 but with the caveat that values SHOULD be capped somewhere in the days or weeks, suggesting 7 days (604,800 seconds) as the cap.

Which is a long winded way to suggest that Ttl should probably have a cap method that simply caps it at those 7 days – perhaps also a constant CAP with that value.

CrabNejonas · 2023-04-25T12:31:50Z

There’s a weird quirk in RFC 1034 (or 1035) where is mistakenly declared TTL at one point to be a signed number. RFC 2181 later made it officially unsigned but said that any value with the most significant bit said should be treated as zero. Years later, RFC 8767 suggested that that was a bad idea and made the TTL properly u32 but with the caveat that values SHOULD be capped somewhere in the days or weeks, suggesting 7 days (604,800 seconds) as the cap.

Which is a long winded way to suggest that Ttl should probably have a cap method that simply caps it at those 7 days – perhaps also a constant CAP with that value.

That's incredibly weird hahaha, I didn't know. But yeah that makes sense 👍

CrabNejonas · 2023-04-25T13:59:29Z

Alright, I added doc comments + examples to all methods mirroring std::time::Duration. A few things of note:

Ttl doesn't implement From<Duration> as the conversion is lossy (Duration can represent sub-second values and stores seconds in u64) so conversion is exposed as Ttl::from_duration_lossy it does however implement Into<Duration> because that conversion can be done without an issue.

In examples I used core::time::Duration because the rest of the crate has to work in no_std, but maybe this is confusing?

partim · 2023-04-26T08:17:56Z

I think having the examples require the std feature and then use that us okay. Probably less confusing, indeed.

CrabNejonas · 2023-04-26T22:29:54Z

I think having the examples require the std feature and then use that us okay. Probably less confusing, indeed.

yep, done!

partim

Again, thank you for this very nice PR! Also thank you for the quick turn-around – this allows us to include it in the soon-to-be-released 0.8.0.

@CrabNejonas

Breaking Changes * The minimal required Rust version is now 1.65. ([#160]) * The `random` feature has been dropped in favour of using `rand`. ([#204]) * The generic octets foundation has been moved to a new crate *[octseq]* and completely revamped with Generic Associated Types stabilized in Rust 1.65. This required changes all over the code but, hopefully, should result in relatively few changes when using the crate. ([#160]) * The range, slice, and split methods on the domain name types have changed. They have been merge into a single method taking ranges – except for those on `Dname` that require type changes. The split methods now take references and don’t change `self` anymore. ([#160]) * The `Parse`, `Compose`, and `Scan` traits have been demoted to mere extension traits for foreign types (primarily the built-in integers, so that you can do things like `u16::parse`). All other types now simply have methods matching the patterns. Where generics are necessary, dedicated traits have been added. E.g., there now are `ParseRecordData` and `ComposeRecordData` traits that are implemented by all record data types. ([#160]) * The `Deref` and `DerefMut` impls have been removed for most types that had them to follow guidance that they are exclusively for use by pointer types – which none of them are. `len` and `is_empty` methods have been added where appropriate, additional methods may be added. ([#205]) * Various functions and methods of the `tsig` module now expect the current time as an argument to allow use of the module in a no-std environment. ([#152]) * Parsing of representation format and zonefiles has been completely re-written. ([#142], based on work in [#109] by [Martin Fischer]) * All types that wrap an octets sequence only now allow unsized octets sequence types. They all have an associated function `from_slice` to create a reference to a value wrapping an (unsized) octets slice and method `for_slice` that converts a `&self` into such a reference. Where the latter already existed but returned a value wrapping a `&[u8]` (e.g., `Dname<_>` and `Message<_>`, the return type has changed accordingly. ([#168]) * Removed `CharStr::from_bytes`. Use `CharStr::from_octets` instead. ([#168]) * `Message::from_octets` now returns a new error type `ShortMessage`. ([#168]) * Dropped `Deref` impls for `Dname<_>`, `RelativeDname<_>`. ([#168]) * Renamed `opt::KeyTag::new` to `opt::KeyTag::from_octets`. ([#168]) * Renamed `rdata::Txt::try_from_slice` to `build_from_slice`. ([#168]) * The `new` method of the following record data types now check whether the wire format representation of the record data is too long and thus returns a result: `Tsig<_, _>`, `Dnskey<_>`, `Rrsig<_, _>`, `Ds<_>`, `Cdnskey<_>`, `Cds<_>`. ([#169]) * The `new` function for `rdata::Null<_>` has been replaced with a `from_octets` and `from_slice` pair. The `Deref` impl was removed. ([#169]) * The `rdata::svcb` module has been refactored to work in the same way as other type-length-value constructs. The names of types, methods, and functions have changed both to match the usual nomenclature as well as to match the terms used in the SVCB draft. ([#176]) * The `base::iana::SvcbParamKey` type has been renamed to `SvcParamKey` to match the terms used in the SVCB draft. ([#176]) * The `TcpKeepalive` option has been changed to use an `Option<u16>` as its data and allow for an empty option in accordance with the RFC. ([#185]) * Renamed the sub-modules of `rdata` that contain record data types to use a name derived from their content rather than their RFC number – with the exception of `rdata::rfc1035`. ([#189]) * Renamed the sub-modules of `base::opt` that contain option data types to use short-hand names rather than their RFC number. ([#190]) * TTL values are now using a newtype `base::record::Ttl` that wraps the raw `u32` and improves conversions. ([#202] by [@CrabNejonas]) * Changes all option data types to ensure their wire format is at most 65,535 octets long. This requires changing the signatures of some creator functions. Their naming scheme and signatures are also changed to follow the pattern established with record data. ([#193]) * Renamed `UnknownOptData::from_octets` to `new` and return a result. ([#193]) * Completely redesigns DNS cookie options, adding support for standard server cookies introduced in RFC 9018. ([#193]) * Change the type of `ExtendedError`’s text to `Str<Octs>` and change the return type of `set_text` to `()`. ([#193]) * Changed the type `TcpKeepalive`’s content to a newtype `IdleTimeout` to make it easier to convert to and from durations. ([#193]) * Changes Padding to just contain the padding octets and drop `PaddingMode`. Instead, the methods on `OptBuilder` should be used to add padding. ([#193]) New * `Display` impls are now available for all EDNS0 options. ([#157]) * Adds a `FromStr` implementation and related functions to `RelativeDname`. ([#177]) * Add a `Debug` impl to `base::message::Message` so it can be unwrapped etc. ([#199]) * New methods `make_canonical` on `Dname` and `RelativeDname` that convert the name into its canonical, i.e., lowercase form. Similarly, new methods `ToDname::to_canonical_dname` and `ToRelativeDname::to_canonical_relative_dname` that produce new canonical names. ([#200]) * Added a `MAX_LEN` constant to various types that wrap length-limited octets sequences. ([#201] by [@CrabNejonas])

@CrabNejonas

Breaking Changes * The minimal required Rust version is now 1.65. ([#160]) * The `random` feature has been dropped in favour of using `rand`. ([#204]) * The generic octets foundation has been moved to a new crate *[octseq]* and completely revamped with Generic Associated Types stabilized in Rust 1.65. This required changes all over the code but, hopefully, should result in relatively few changes when using the crate. ([#160]) * The range, slice, and split methods on the domain name types have changed. They have been merge into a single method taking ranges – except for those on `Dname` that require type changes. The split methods now take references and don’t change `self` anymore. ([#160]) * The `Parse`, `Compose`, and `Scan` traits have been demoted to mere extension traits for foreign types (primarily the built-in integers, so that you can do things like `u16::parse`). All other types now simply have methods matching the patterns. Where generics are necessary, dedicated traits have been added. E.g., there now are `ParseRecordData` and `ComposeRecordData` traits that are implemented by all record data types. ([#160]) * The `Deref` and `DerefMut` impls have been removed for most types that had them to follow guidance that they are exclusively for use by pointer types – which none of them are. `len` and `is_empty` methods have been added where appropriate, additional methods may be added. ([#205]) * Various functions and methods of the `tsig` module now expect the current time as an argument to allow use of the module in a no-std environment. ([#152]) * Parsing of representation format and zonefiles has been completely re-written. ([#142], based on work in [#109] by [Martin Fischer]) * All types that wrap an octets sequence only now allow unsized octets sequence types. They all have an associated function `from_slice` to create a reference to a value wrapping an (unsized) octets slice and method `for_slice` that converts a `&self` into such a reference. Where the latter already existed but returned a value wrapping a `&[u8]` (e.g., `Dname<_>` and `Message<_>`, the return type has changed accordingly. ([#168]) * Removed `CharStr::from_bytes`. Use `CharStr::from_octets` instead. ([#168]) * `Message::from_octets` now returns a new error type `ShortMessage`. ([#168]) * Dropped `Deref` impls for `Dname<_>`, `RelativeDname<_>`. ([#168]) * Renamed `opt::KeyTag::new` to `opt::KeyTag::from_octets`. ([#168]) * Renamed `rdata::Txt::try_from_slice` to `build_from_slice`. ([#168]) * The `new` method of the following record data types now check whether the wire format representation of the record data is too long and thus returns a result: `Tsig<_, _>`, `Dnskey<_>`, `Rrsig<_, _>`, `Ds<_>`, `Cdnskey<_>`, `Cds<_>`. ([#169]) * The `new` function for `rdata::Null<_>` has been replaced with a `from_octets` and `from_slice` pair. The `Deref` impl was removed. ([#169]) * The `rdata::svcb` module has been refactored to work in the same way as other type-length-value constructs. The names of types, methods, and functions have changed both to match the usual nomenclature as well as to match the terms used in the SVCB draft. ([#176]) * The `base::iana::SvcbParamKey` type has been renamed to `SvcParamKey` to match the terms used in the SVCB draft. ([#176]) * The `TcpKeepalive` option has been changed to use an `Option<u16>` as its data and allow for an empty option in accordance with the RFC. ([#185]) * Renamed the sub-modules of `rdata` that contain record data types to use a name derived from their content rather than their RFC number – with the exception of `rdata::rfc1035`. ([#189]) * Renamed the sub-modules of `base::opt` that contain option data types to use short-hand names rather than their RFC number. ([#190]) * TTL values are now using a newtype `base::record::Ttl` that wraps the raw `u32` and improves conversions. ([#202] by [@CrabNejonas]) * Changes all option data types to ensure their wire format is at most 65,535 octets long. This requires changing the signatures of some creator functions. Their naming scheme and signatures are also changed to follow the pattern established with record data. ([#193]) * Renamed `UnknownOptData::from_octets` to `new` and return a result. ([#193]) * Completely redesigns DNS cookie options, adding support for standard server cookies introduced in RFC 9018. ([#193]) * Change the type of `ExtendedError`’s text to `Str<Octs>` and change the return type of `set_text` to `()`. ([#193]) * Changed the type `TcpKeepalive`’s content to a newtype `IdleTimeout` to make it easier to convert to and from durations. ([#193]) * Changes Padding to just contain the padding octets and drop `PaddingMode`. Instead, the methods on `OptBuilder` should be used to add padding. ([#193]) New * `Display` impls are now available for all EDNS0 options. ([#157]) * Adds a `FromStr` implementation and related functions to `RelativeDname`. ([#177]) * Add a `Debug` impl to `base::message::Message` so it can be unwrapped etc. ([#199]) * New methods `make_canonical` on `Dname` and `RelativeDname` that convert the name into its canonical, i.e., lowercase form. Similarly, new methods `ToDname::to_canonical_dname` and `ToRelativeDname::to_canonical_relative_dname` that produce new canonical names. ([#200]) * Added a `MAX_LEN` constant to various types that wrap length-limited octets sequences. ([#201] by [@CrabNejonas])

CrabNejonas added 2 commits April 24, 2023 15:34

fmt & fixes

8a36487

refactor: use custom duration type

213f35c

fix tests

a0ca9f4

use core instead of std

22cec01

partim requested changes Apr 25, 2023

View reviewed changes

src/base/record.rs Outdated Show resolved Hide resolved

src/base/record.rs Outdated Show resolved Hide resolved

src/base/wire.rs Outdated Show resolved Hide resolved

src/base/wire.rs Outdated Show resolved Hide resolved

src/rdata/rfc1035.rs Outdated Show resolved Hide resolved

CrabNejonas added 7 commits April 25, 2023 14:40

return by value instead of ref

5bc3cff

make parse and compose methods instead of trait impls

c59f8de

add proper conversion methods

8f4f343

fixes

26c1e98

add documentation

7e1f5c1

fix tests, fmt & clippy

d91c302

add Into<Duration> impl

efcaf66

CrabNejonas added 2 commits April 27, 2023 00:27

fixes

f3300a6

fmt

6905b9e

Delete settings.json

0a9a78e

partim self-requested a review April 28, 2023 08:15

partim approved these changes Apr 28, 2023

View reviewed changes

partim merged commit 5350da9 into NLnetLabs:main Apr 28, 2023

CrabNejonas deleted the refactor/duration branch April 28, 2023 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use `Duration` instead of `u32` #202

refactor: use `Duration` instead of `u32` #202

CrabNejonas commented Apr 24, 2023

partim commented Apr 25, 2023

CrabNejonas commented Apr 25, 2023

partim commented Apr 25, 2023

partim commented Apr 25, 2023

CrabNejonas commented Apr 25, 2023

CrabNejonas commented Apr 25, 2023

partim commented Apr 26, 2023

CrabNejonas commented Apr 26, 2023

partim left a comment

refactor: use Duration instead of u32 #202

refactor: use Duration instead of u32 #202

Conversation

CrabNejonas commented Apr 24, 2023

partim commented Apr 25, 2023

CrabNejonas commented Apr 25, 2023

partim commented Apr 25, 2023

partim commented Apr 25, 2023

CrabNejonas commented Apr 25, 2023

CrabNejonas commented Apr 25, 2023

partim commented Apr 26, 2023

CrabNejonas commented Apr 26, 2023

partim left a comment

Choose a reason for hiding this comment

refactor: use `Duration` instead of `u32` #202

refactor: use `Duration` instead of `u32` #202