Conform to RFC 3396 & RFC 2131 wrt Options

[hugelgupf]

Removes AddOption and GetOption.

RFC 2131 specifies that options may only appear once (Section 4.1). If
an option does appear more than once, its byte values must be
concatenated.

RFC 3396 further specifies that to send options longer than 255 bytes,
one option may be split into multiple option codes, which must be
concatenated back together by the receiver.

Both of these are concerned with the byte representation of options.
Fact is, based on both RFCs one can say that an option may only appear
once, but may be composed of multiple values. (Like the multiple IP options.)

Because an option may appear only once logically in any case, we remove
the AddOption and GetOption functions and leave only UpdateOption and
GetOneOption.

Also remove all additions & checks of the End option - the marshaling
and unmarshaling code is exclusively responsible for that now.

(will rebase after #226 is merged.)

The full RFC 3396 is not supported at the moment, as the Overload option is not respected. (Who TF even uses that.)

[pmazzini]

This looks good to me, thanks for the cleanup and improvements.

dhcpv4/ztpv4/ztp_test.go will need to be updated as well since it is calling AddOption.

[pmazzini]

Also approving but not merging yet to give @insomniacslk a chance to look at it.

[codecov]

Codecov Report

Merging #228 into master will decrease coverage by 0.48%.
The diff coverage is 94.25%.

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
- Coverage   72.39%   71.91%   -0.49%     
==========================================
  Files          83       83              
  Lines        3688     3642      -46     
==========================================
- Hits         2670     2619      -51     
- Misses        902      905       +3     
- Partials      116      118       +2

Impacted Files	Coverage Δ
dhcpv4/bsdp/option_vendor_specific_information.go	84% <ø> (-0.62%)	⬇️
dhcpv4/dhcpv4.go	66.66% <100%> (-1.67%)	⬇️
dhcpv4/option_parameter_request_list.go	100% <100%> (+9.52%)	⬆️
dhcpv4/modifiers.go	100% <100%> (ø)	⬆️
dhcpv4/types.go	71.42% <66.66%> (ø)	⬆️
dhcpv4/options.go	89.68% <77.77%> (-3.06%)	⬇️
dhcpv4/bsdp/bsdp.go	80.92% <92.59%> (-1.44%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84a6137...cfd564b. Read the comment docs.

[hugelgupf]

rebased :)

[insomniacslk]

Looks good to me, thanks @hugelgupf !

Just one question - what about the Pad option? Per concept of padding I would expect that Pad could be added more than once, but this clashes with what described in the RFC.

[hugelgupf]

Just one question - what about the Pad option? Per concept of padding I would expect that Pad could be added more than once, but this clashes with what described in the RFC.

I think padding should be done by the marshaling routines where necessary. (To pad the packet to its minimum length or to pad the Overload fields.) One more thing for the user not to worry about.

[insomniacslk]

I think padding should be done by the marshaling routines where necessary. (To pad the packet to its minimum length or to pad the Overload fields.) One more thing for the user not to worry about.

Sounds good. This also confirms that after these changes this becomes a production-only library and not allowing anymore scapy-like packet crafting (which is OK, just acknowledging here for the future choices)

[pmazzini]

Most things can still be crafted but you are right, we won't control the padding. For me it is OK too.