insomniacslk / dhcp

DHCPv6 and DHCPv4 packet library, client and server written in Go
BSD 3-Clause "New" or "Revised" License
685 stars 168 forks source link

Option helper for AutoConfigure #525

Closed candlerb closed 4 months ago

candlerb commented 4 months ago

This isn't really a fully-fledged pull request: I'm seeking advice on how best to expose the option AutoConfigure from RFC 2563.

What the proposed patch does is implement a Byte option, based on Uint16 from dhcpv4/option_maximum_dhcp_message_size.go

However, other possibilities would be:

  1. Implement a Bool option
  2. Implement an enumeration which is specific to AutoConfigure - so that for example, the String() method would return "Auto-Configure: DoNotAutoConfigure" or "Auto-Configure: AutoConfigure"
  3. Do nothing and require clients to use GenericOption directly. However there could be a small helper for setting this generic option, along the lines of OptClientIdentifier() in dhcpv4/option_misc.go

Based on the registry, some other examples of single-byte DHCP options are:

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 96.77419% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 73.35%. Comparing base (3c11703) to head (45ea332).

Files Patch % Lines
dhcpv4/option_autoconfigure.go 96.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #525 +/- ## ========================================== + Coverage 73.21% 73.35% +0.14% ========================================== Files 80 81 +1 Lines 5073 5104 +31 ========================================== + Hits 3714 3744 +30 - Misses 1216 1217 +1 Partials 143 143 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

candlerb commented 4 months ago

Ready for review

candlerb commented 4 months ago

I have changed the AutoConfigure() helper to return (value, bool) instead of (value, error). I do need this one for coredhcp.

Please could you kick off the CI, and if it's happy, I'm good to merge this now.