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

dhcpcd: Add support for arp persist defence #273

Merged
merged 1 commit into from
Dec 21, 2023
Merged

dhcpcd: Add support for arp persist defence #273

merged 1 commit into from
Dec 21, 2023

Conversation

pradeep-brightsign
Copy link
Contributor

RFC 5227 recommends 3 ways to deal with address conflict detection.
a) Stop everything.
b) Defend and then stop on fail - this is what dhcpcd currently does.
c) Notify and carry on.

The current change implements the option c. A new option arp_persistdefence has been added and when this is enabled, the a defence is attempted upon a conflict and when that fails, an error is logged on every other conflict within the DEFEND_INTERVAL and the current IP address is retained.

Copy link
Member

@rsmarples rsmarples left a comment

Choose a reason for hiding this comment

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

Please update dhcpcd.conf.5.in with the new option, next to the arp option and bump the date of it as well please.

src/arp.c Outdated
logwarnx("%s: %d second defence failed for %s",
ifp->name, DEFEND_INTERVAL, inet_ntoa(astate->addr));
eloop_timespec_diff(&now, &astate->defend, NULL) <
DEFEND_INTERVAL) {
Copy link
Member

Choose a reason for hiding this comment

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

Newline before the opening curly brace please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

src/arp.c Outdated
DEFEND_INTERVAL) {
if ((ifp->options->options &
DHCPCD_ARP_PERSISTDEFENCE) ==
DHCPCD_ARP_PERSISTDEFENCE) {
Copy link
Member

Choose a reason for hiding this comment

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

No need for == here - (a & b) will produce a truthy value.
Also, new line before the opening curly brace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

src/arp.c Outdated
if ((ifp->options->options &
DHCPCD_ARP_PERSISTDEFENCE) ==
DHCPCD_ARP_PERSISTDEFENCE) {
logerrx("%s: %d second defence ignored as " \
Copy link
Member

Choose a reason for hiding this comment

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

Scrap this diagnostic.
Just keep the existing diagnostic and early return if DHCPCD_ARP_PERSISTDEFENCE.
We can tell if it's set or not as the address will be kept or dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -510,6 +510,8 @@ adding a new IPv4 address.
.It Ic noarp
Don't send any ARP requests.
This also disables IPv4LL.
.It Ic arp_persistdefence
Keep the IP address even if defense fails upon IP Address conflict.
Copy link
Member

Choose a reason for hiding this comment

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

British spelling of defence please :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

Copy link
Member

@rsmarples rsmarples left a comment

Choose a reason for hiding this comment

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

This is looking good now, just one spelling issue to address and I'll merge it.

@rsmarples
Copy link
Member

Oh yeah, add Fixes #272 at the end of your commit message and it will close the issue automatically and link to it for future reference.

RFC 5227 recommends 3 ways to deal with address conflict detection.
a) Stop everything.
b) Defend and then stop on fail - this is what dhcpcd currently does.
c) Notify and carry on.

The current change implements the option c. A new option arp_persistdefence
has been added and when this is enabled, the a defence is attempted upon a
conflict and when that fails, an error is logged on every other conflict
within the DEFEND_INTERVAL and the current IP address is retained.

Fixes #272
@rsmarples rsmarples merged commit e65e82a into NetworkConfiguration:master Dec 21, 2023
@rsmarples
Copy link
Member

Thanks for the patch!

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

2 participants