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

IsOptionRequested does not work with GenericOptionCode #511

Closed candlerb closed 3 months ago

candlerb commented 10 months ago

I was expecting that if I added a GenericOptionCode to a Parameter Request List, then IsOptionRequested() would be able to detect it - but it does not.

Here is a failing test case:

--- a/dhcpv4/dhcpv4_test.go
+++ b/dhcpv4/dhcpv4_test.go
@@ -378,6 +378,17 @@ func TestIsOptionRequested(t *testing.T) {
        require.True(t, pkt.IsOptionRequested(OptionDomainNameServer))
 }

+func TestIsGenericOptionRequested(t *testing.T) {
+       pkt, err := New()
+       require.NoError(t, err)
+
+       var optionV6OnlyPreferred GenericOptionCode = 108
+       pkt.UpdateOption(OptParameterRequestList(OptionBroadcastAddress, optionV6OnlyPreferred))
+       require.True(t, pkt.IsOptionRequested(OptionBroadcastAddress))
+       require.False(t, pkt.IsOptionRequested(OptionSwapServer))
+       require.True(t, pkt.IsOptionRequested(optionV6OnlyPreferred), "Did not detect generic option in %v", pkt.ParameterRequestList())
+}
+
 // TODO
 //      test Summary() and String()
 func TestSummary(t *testing.T) {

(This example uses option code 108, from RFC8925, which is not provided as a predefined OptionCode)

pmazzini commented 3 months ago

(This example uses option code 108, from RFC8925, which is not provided as a predefined OptionCode)

It is in here: https://github.com/insomniacslk/dhcp/blob/master/dhcpv4/types.go#L242

candlerb commented 3 months ago

I got it added in #524 :-)

But I think the point about detecting GenericOptionCodes still stands.

pmazzini commented 3 months ago

I think this is because you are comparing optionCode against GenericOptionCode, different types.

https://github.com/insomniacslk/dhcp/blob/master/dhcpv4/dhcpv4.go#L486

candlerb commented 3 months ago

Certainly it may be a question of expectations and/or documentation. However, I note that:

Therefore, it was my expectation that this function would work for any argument implementing the OptionCode interface (and AFAIK there's no other way to generate a custom optionCode value, since it's a private type).

What if the comparison were changed to o.Code() == requested.Code() ?