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

plugins/netmask: Don't panic on invalid netmask #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

purpleidea
Copy link

It looks like the error handling for this plugin is borked. I noticed
this while reading this code during my attempt to implement a DHCP
server. This also clarifies the error messages, since they were either
innaccurate, or duplicated which would make it impossible to tell which
code path the error actually came from.

NOTE: I didn't test this code, but it seems pretty trivial. I hope you have CI, but since this bug got through, then maybe not?

It looks like the error handling for this plugin is borked. I noticed
this while reading this code during my attempt to implement a DHCP
server. This also clarifies the error messages, since they were either
innaccurate, or duplicated which would make it impossible to tell which
code path the error actually came from.
@Natolumin
Copy link
Member

There's CI, but not everything has good test coverage, especially not error checks.

Please signoff your commit (read https://developercertificate.org/ then git commit --amend --signoff and force-push).
The test failure looks like an infra issue on travis's side, it should retry next time you push

Otherwise, looks fine, we can merge once that's done

@insomniacslk
Copy link
Member

I've retried the failing CI job and it works just fine. @purpleidea can you update your commit with DCO as explained above? Thanks!

@insomniacslk
Copy link
Member

Ping @purpleidea , still interested in merging this PR?

@skoef
Copy link
Contributor

skoef commented Nov 29, 2021

I guess this is superseded by #144

@purpleidea
Copy link
Author

If you think it still needs fixing lmk and I can rebase. Otherwise close. Cheers

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.

None yet

4 participants