check the length of lists given in options before writing them #48

Merged
merged 3 commits into from Mar 29, 2017

Conversation

Projects
None yet
2 participants
@yomimono
Member

yomimono commented Mar 29, 2017

/cc @haesbaert - I don't think this is too gory, what do you think?

@yomimono

This comment has been minimized.

Show comment
Hide comment
@yomimono

yomimono Mar 29, 2017

Member

I should make this parameter optional since there's only one call where it's anything other than 1, standby for force push...

Member

yomimono commented Mar 29, 2017

I should make this parameter optional since there's only one call where it's anything other than 1, standby for force push...

@yomimono yomimono requested a review from haesbaert Mar 29, 2017

@haesbaert

This comment has been minimized.

Show comment
Hide comment
@haesbaert

haesbaert Mar 29, 2017

Member

Hmm I think that's doable, but we really need to check them all before default to min=1.

Can you split the bugfixes/whitespace fixes into a different commit, that wrong code number is a good find.

If after we check them all, it might be that the option could be something like emptyok=true instead of giving a min_len, I suspect we will only use min_len=0.

Since we are here, we might consider minimum length checks on single options too, like strings

Hah, only now I saw that the inverse is already done in options_of_buf.

Member

haesbaert commented Mar 29, 2017

Hmm I think that's doable, but we really need to check them all before default to min=1.

Can you split the bugfixes/whitespace fixes into a different commit, that wrong code number is a good find.

If after we check them all, it might be that the option could be something like emptyok=true instead of giving a min_len, I suspect we will only use min_len=0.

Since we are here, we might consider minimum length checks on single options too, like strings

Hah, only now I saw that the inverse is already done in options_of_buf.

@yomimono

This comment has been minimized.

Show comment
Hide comment
@yomimono

yomimono Mar 29, 2017

Member
Member

yomimono commented Mar 29, 2017

@haesbaert

This comment has been minimized.

Show comment
Hide comment
@haesbaert

haesbaert Mar 29, 2017

Member

I think the diff is fine, we're already enforcing min_len for every list option on the other way. So we should enforce when encoding too.

Just split them up and I'll merge :D.

Thanks for this !

Member

haesbaert commented Mar 29, 2017

I think the diff is fine, we're already enforcing min_len for every list option on the other way. So we should enforce when encoding too.

Just split them up and I'll merge :D.

Thanks for this !

@haesbaert

This comment has been minimized.

Show comment
Hide comment
@haesbaert

haesbaert Mar 29, 2017

Member

This actually prompted me to have a look at get_8_list and get_16_list, they don't enforce min_len, I'll write a diff.

Member

haesbaert commented Mar 29, 2017

This actually prompted me to have a look at get_8_list and get_16_list, they don't enforce min_len, I'll write a diff.

@haesbaert haesbaert merged commit f25a15e into mirage:master Mar 29, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@yomimono yomimono referenced this pull request in moby/vpnkit Apr 7, 2017

Merged

don't add DHCP options with empty lists #208

@stedolan stedolan referenced this pull request in stedolan/crowbar May 24, 2017

Open

Bugs found with Crowbar #2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment