dhcpv4: simplify option parsing. #224

hugelgupf · 2019-01-09T23:33:13Z

option's codes and lengths were being parsed twice: once in ParseOption
and once in each option type's Parse implementation. Consolidate such
that it only happens once.

Additionally, only pass data to options that they should parse -- we
know the length before the Parse function is called, so the option only
gets to see the data it needs to see.

Also, use uio.Lexer to simplify parsing code in general. Easier to read
and reason about.

hugelgupf · 2019-01-09T23:33:36Z

This will have very minor merge conflicts with #223. I'll resolve depending on whichever gets approved & submitted first.

codecov · 2019-01-10T00:03:11Z

Codecov Report

Merging #224 into master will decrease coverage by 1.01%.
The diff coverage is 97.28%.

@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
- Coverage   73.96%   72.95%   -1.02%     
==========================================
  Files          84       84              
  Lines        4118     3797     -321     
==========================================
- Hits         3046     2770     -276     
+ Misses        935      912      -23     
+ Partials      137      115      -22

Impacted Files	Coverage Δ
dhcpv4/bsdp/bsdp_option_generic.go	`100% <100%> (ø)`	⬆️
dhcpv4/option_requested_ip_address.go	`100% <100%> (ø)`	⬆️
dhcpv4/option_generic.go	`100% <100%> (+7.4%)`	⬆️
dhcpv4/options.go	`94.23% <100%> (+4.48%)`	⬆️
dhcpv4/option_maximum_dhcp_message_size.go	`100% <100%> (ø)`	⬆️
dhcpv4/option_broadcast_address.go	`100% <100%> (ø)`	⬆️
dhcpv4/option_ip_address_lease_time.go	`100% <100%> (ø)`	⬆️
dhcpv4/bsdp/bsdp_option_version.go	`100% <100%> (ø)`	⬆️
dhcpv4/option_message_type.go	`100% <100%> (ø)`	⬆️
dhcpv4/option_root_path.go	`100% <100%> (ø)`	⬆️
... and 28 more

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 108ed92...2ba543b. Read the comment docs.

get9

Couple of nits/questions, nothing blocking.

get9 · 2019-01-10T01:03:56Z

dhcpv4/bsdp/bsdp_option_default_boot_image_id.go

 )

+// OptDefaultBootImageID contains the selected boot image ID.


This should be moved back down.

That's not how Go comments work?

The diff may make it weird to read, but this is

// OptDefaultBootImageID contains the selected boot image ID. // // Implements the BSDP option default boot image ID, which tells the client // which image is the default boot image if one is not selected. type OptDefaultBootImageID ...

dhcpv4/bsdp/bsdp_option_machine_name.go

get9 · 2019-01-10T01:06:15Z

dhcpv4/bsdp/option_vendor_specific_information.go

@@ -11,20 +9,17 @@ import (
 // OptVendorSpecificInformation encapsulates the BSDP-specific options used for
 // the protocol.
 type OptVendorSpecificInformation struct {
-	Options []dhcpv4.Option
+	Options dhcpv4.Options


Not sure that I like this particular type alias - what's wrong with just having it as a slice? it's more clear that it's a slice of a struct/interface, rather than having to look at what a dhcpv4.Options is.

I also asked this in the original PR, and IIRC @hugelgupf replied (not here, offline) that a rework that doesn't require this new type was ongoing. @hugelgupf is this still the case?

So here's the deal. You've got the []dhcpv4.Option type in a couple of places: DHCPv4, OptRelayAgentInfo, and here.

A dhcpv4.Options type can be the one place to implement the following:

func (Options) Get(...) func (Options) Has(...) func (Options) Add(...) func (Options) Update(...)

rather than reimplementing GetOption and GetOneOption on multiple types and having the OptionGetter interface, just implement it on Options and spread Options around.

I.e. there is no need to have both

dhcp/dhcpv4/bsdp/option_vendor_specific_information.go

Line 135 in 108ed92

func (o *OptVendorSpecificInformation) GetOption(code dhcpv4.OptionCode) []dhcpv4.Option {

and

dhcp/dhcpv4/dhcpv4.go

Line 366 in 108ed92

func (d *DHCPv4) GetOption(code OptionCode) []Option {

and eventually you'll start replicating HasOption and UpdateOption and AddOption to OptVendorSpecificInformation as well as the relay agent stuff... so an Options type saves you that work.

yeah I get how this works, and in an isolated context it makes perfectly sense from a compactness perspective. I think it introduces a bit more confusion, but it's probably acceptable compared to the benefits of a better / more compact interface. As long as we use always Options instead of []Option I guess this is OK. If we don't like it at any stage it's always possible to go back. Let's go with Options now

dhcpv4/options.go

insomniacslk

Thanks! This PR greatly increases the readability and reduces the complexity. Just a few things to be clarified and potentially changed, as per the inline comments

insomniacslk · 2019-01-10T15:41:06Z

dhcpv4/bsdp/option_vendor_specific_information.go

@@ -11,20 +9,17 @@ import (
 // OptVendorSpecificInformation encapsulates the BSDP-specific options used for
 // the protocol.
 type OptVendorSpecificInformation struct {
-	Options []dhcpv4.Option
+	Options dhcpv4.Options


I also asked this in the original PR, and IIRC @hugelgupf replied (not here, offline) that a rework that doesn't require this new type was ongoing. @hugelgupf is this still the case?

insomniacslk · 2019-01-10T15:48:06Z

dhcpv4/bsdp/types.go

 // describing what they are.
-var OptionCodeToString = map[dhcpv4.OptionCode]string{
+var optionCodeToString = map[dhcpv4.OptionCode]string{


with this becoming private, how does a user convert an option code to its name if they want to? Only OptionGeneric seems to allow a form of access to this map

OptionCode.String()

Argh, yeah, for bsdp this is kinda weird. There's a commit way down the line to make the printing right, but for now, this is a weird thing to expose.

I suppose I should try to fix this in this PR. Let me see what I can do.

Actually, in this PR it doesn't matter yet -- each Option in BSDP prints its own option name statically anyway: https://github.com/insomniacslk/dhcp/pull/224/files#diff-d173dc6740eaa1ad94d4f356b77a647eR32

I hate BSDP -.-

dhcpv4/dhcpv4.go

option's codes and lengths were being parsed twice: once in ParseOption and once in each option type's Parse implementation. Consolidate such that it only happens once. Additionally, only pass data to options that they should parse -- we know the length before the Parse function is called, so the option only gets to see the data it needs to see. Also, use uio.Lexer to simplify parsing code in general. Easier to read and reason about.

dhcpv4/dhcpv4.go

pmazzini · 2019-01-10T21:49:41Z

Thanks! This fixes #22. It looks good to me.

hugelgupf · 2019-01-10T22:25:28Z

@pmazzini sry, force pushed to clean something up, mind approving again?

insomniacslk · 2019-01-10T23:01:33Z

Thanks a lot for this PR @hugelgupf !

hugelgupf mentioned this pull request Jan 9, 2019

dhcpv4: nicer parsing of messages. #222

Closed

hugelgupf force-pushed the parsing-test branch from 1cf1137 to ef2cb2d Compare January 9, 2019 23:54

insomniacslk requested review from pmazzini, get9 and insomniacslk January 10, 2019 00:14

hugelgupf force-pushed the parsing-test branch from ef2cb2d to 6d742d2 Compare January 10, 2019 00:55

get9 reviewed Jan 10, 2019

View reviewed changes

insomniacslk reviewed Jan 10, 2019

View reviewed changes

hugelgupf force-pushed the parsing-test branch from 6d742d2 to 3d2d1c7 Compare January 10, 2019 21:16

pmazzini reviewed Jan 10, 2019

View reviewed changes

dhcpv4/dhcpv4.go Outdated Show resolved Hide resolved

hugelgupf force-pushed the parsing-test branch from 3d2d1c7 to e7673a0 Compare January 10, 2019 21:29

dhcpv4: remote OptionGetter interface in favor of Options type.

2ba543b

hugelgupf force-pushed the parsing-test branch from e7673a0 to 2ba543b Compare January 10, 2019 22:21

pmazzini approved these changes Jan 10, 2019

View reviewed changes

pmazzini requested review from get9 and insomniacslk January 10, 2019 22:32

hugelgupf mentioned this pull request Jan 10, 2019

dhcpv4: simplify option marshaling #225

Merged

insomniacslk merged commit 512011c into insomniacslk:master Jan 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dhcpv4: simplify option parsing. #224

dhcpv4: simplify option parsing. #224

hugelgupf commented Jan 9, 2019

hugelgupf commented Jan 9, 2019 •

edited

Loading

codecov bot commented Jan 10, 2019 •

edited

Loading

get9 left a comment

get9 Jan 10, 2019

hugelgupf Jan 10, 2019

get9 Jan 10, 2019

insomniacslk Jan 10, 2019

hugelgupf Jan 10, 2019

hugelgupf Jan 10, 2019

insomniacslk Jan 10, 2019

insomniacslk left a comment

insomniacslk Jan 10, 2019

insomniacslk Jan 10, 2019

hugelgupf Jan 10, 2019

hugelgupf Jan 10, 2019

hugelgupf Jan 10, 2019

hugelgupf Jan 10, 2019

insomniacslk Jan 10, 2019

pmazzini commented Jan 10, 2019

hugelgupf commented Jan 10, 2019

insomniacslk commented Jan 10, 2019

		)

		// OptDefaultBootImageID contains the selected boot image ID.

dhcpv4: simplify option parsing. #224

dhcpv4: simplify option parsing. #224

Conversation

hugelgupf commented Jan 9, 2019

hugelgupf commented Jan 9, 2019 • edited Loading

codecov bot commented Jan 10, 2019 • edited Loading

Codecov Report

get9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

insomniacslk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmazzini commented Jan 10, 2019

hugelgupf commented Jan 10, 2019

insomniacslk commented Jan 10, 2019

hugelgupf commented Jan 9, 2019 •

edited

Loading

codecov bot commented Jan 10, 2019 •

edited

Loading