Skip to content

move toward option print/parse parity #50

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

Merged
merged 1 commit into from
Apr 1, 2017

Conversation

yomimono
Copy link
Contributor

Without this patch, buf_of_pkt will emit options that pkt_of_buf discards. This patch is an attempt to use the existing parser functions to implement the options which buf_of_pkt will output at user request.

Copy link
Member

@haesbaert haesbaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reads good to me except the min_len which is incorrect.

lib/dhcp_wire.ml Outdated
| 86 -> take (Nds_tree_name (get_string ()))
| 87 -> take (Nds_context (get_string ()))
| 88 -> take (Bcmcs_controller_domain_name_list (get_string ()))
| 89 -> take (Bcmcs_controller_ipv4_addrs (get_ip_list ~min_len:1 ()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min_len in this function is not List.length, it is the minimum size of the object in bytes, so the correct should be the default min_len:4, which is one ip.

lib/dhcp_wire.ml Outdated
| 89 -> take (Bcmcs_controller_ipv4_addrs (get_ip_list ~min_len:1 ()))
| 90 -> take (Authentication (get_string ()))
| 91 -> take (Client_last_transaction_time (get_32 ()))
| 92 -> take (Associated_ips (get_ip_list ~min_len:1 ()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! min_len is correctly specified in the default for get_ip_list, so I'll just remove it -- not sure what I was thinking here.

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