Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move implementation of DHCPv4 options into a macro #51

Merged

Conversation

HayleyDeckers
Copy link
Contributor

@HayleyDeckers HayleyDeckers commented Jan 4, 2023

This PR adds a proc-macro for defining DHCPv4 options as discussed in #43 , making it easier to add more options and keeping the definitions of DhcpOption and OptionCode on a single line.

I tried keeping the macro itself relatively straightforward using simple string manipulation. There's probably room for improvement but if so, hopefully this can serve as a good starting point.

Things to note:

  • There was a minor copy-paste error in the DhcpOption docstring of Option 49 XDisplayManager.
  • The BulkLeaseQuery* items of DhcpOption were prefixed with BulkLeaseQuery but they were not prefixed in OptionCode (i.e. DhcpOption::BulkLeaseQueryStatusCode mapped to OptionCode::StatusCode), this macro prefixes both of them with BulkLeaseQuery, in line with all the other options and keep the macro simple. Technically, the RFC refers to "Bulk leasequeries" so perhaps it should be BulkLeasequery with the Q in lowercase.
  • Does not include the TFTP related options from my other PR, should include all options from master including ClasslessStaticRoute.

@leshow
Copy link
Collaborator

leshow commented Jan 9, 2023

Thank you! This is a good start! Long term, I think I'd like to avoid String output in favor of syn/quote. We don't need to do that on this PR though.

The only breaking change here is the OptionCode::BulkLeaseQuery* types having that prefix added? I'm trying to keep track of breaking changes in the CHANGELOG file. Eventually, we'll try to not make any more breaking changes to type names, but things are still in flux...

@HayleyDeckers
Copy link
Contributor Author

The only breaking change here is the OptionCode::BulkLeaseQuery* types having that prefix added?

I believe so. It is possible to add a special-case to the macro to avoid this, but it's more consistent this way.

@leshow
Copy link
Collaborator

leshow commented Jan 10, 2023

Cool! Can you rebase, or delete the Cargo.lock file? I will merge afterwards.

@leshow leshow merged commit 406cd43 into bluecatengineering:master Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants