Fix DHCP option order not being preserved

[f3rn0s]

Fixes #75 by introducing a data-structure that will preserve insertion order.

[leshow]

Just to be clear, removing the manual insert of End fixes your issue where the encoder did not produce a valid message right? It's just not going to be byte-by-byte the same because the option order will be different?

fyi, the v6 options use an array because it's possible to have multiple options with the same opt code. The items are stored sorted based on the underlying u16 (mostly so they can be accessed with a binary search), it does not preserve insertion order though.

I'm not sure I want to preserve the insertion order unless there's a really good reason. If we're going to break backwards compatibility, I'm leaning towards the btree I think.

[f3rn0s]

Sorry for the delay on this. I'm unsure why this would break backwards compatibility.

It's giving 100% test passing on my machine, it looked like last time was a serde issue, so hopefully by explicitly enabling the serde feature of IndexMap this won't be a problem.

Also imo I think that a protocol encoding library should be byte-by-byte accurate across executions.

[stappersg]

On Wed, Oct 09, 2024 at 09:58:54PM -0700, f3rn0s wrote: Also imo I think that a protocol encoding library should be byte-by-byte accurate between runs.

I think that it is good thing if server replies to options in the same order as original requested by the client. ( In other words: Do not assume that both ends can deal with a reshuffled option order. ) Groeten Geert Stappers -- Silence is hard to parse

[leshow]

@f3rn0s the iterator type makes it into the public API in IntoIter, that's the spot I'm concerned about the breaking change

Have either of you seen anything in RFC's which implied an option order?

I lean more towards btreemap because it's in the standard lib, requires no dependencies, and satisfies your concern of producing the same byte-by-byte output.

[coveralls]

Pull Request Test Coverage Report for Build 11267679355

Details

• 2 of 3 (66.67%) changed or added relevant lines in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-0.1%) to 52.428%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/v4/options.rs	2	3	66.67%

Files with Coverage Reduction	New Missed Lines	%
src/v6/options.rs	1	54.78%

Totals
Change from base Build 9422513589:	-0.1%
Covered Lines:	1598
Relevant Lines:	3048

---

💛 - Coveralls

[f3rn0s]

@leshow I opted for IndexMap since that preserves insertion order over just a consistent, but not necessarily visible/modifiable, order.

There was no reference to ordering in RFC 2131 or 2132, but it would be nice to have granular control in case we came across a server or client implementation that has weird requirements.