Page MenuHomeFreeBSD
Differential D2581

bootpd: add support for disabling ARP cache modifications
ClosedPublic
Actions

Authored by feld on May 19 2015, 1:25 PM.
Tags
None
Referenced Files
F106657032: D2581.diff
Fri, Jan 3, 12:10 PM
F106621861: D2581.id8697.diff
Thu, Jan 2, 9:27 PM
F106618345: D2581.id8697.diff
Thu, Jan 2, 8:06 PM
Unknown Object (File)
Wed, Dec 25, 4:21 AM
Unknown Object (File)
Mon, Dec 16, 8:23 PM
Unknown Object (File)
Tue, Dec 10, 4:40 AM
Unknown Object (File)
Mon, Dec 9, 7:45 PM
Unknown Object (File)
Fri, Dec 6, 1:57 AM
View All Files
Subscribers
allanjude
dan_freebsd.org_obluda.cz
emaste
imp
wblock

Details

Reviewers
dan_freebsd.org_obluda.cz
imp
Group Reviewers
manpages
Commits
rS348066: bootpd: Add an option to skip modifications to the ARP table.
Summary

I ran into someone on the mailing list who has been applying this patch
for 14 years. The PR, #30854 has a patch which almost applies cleanly to
HEAD still so I fixed the minor conflict and added a man page entry.

Seeing how this still applies quite cleanly after 14 years and there's
*someone* out there who desires this functionality it seems harmless to
let it into the tree to save this guy pain every time he updates his
favorite OS. (He has a bunch of other ignored PRs too, but that's a
different conversation)

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

feld updated this revision to Diff 5472.May 19 2015, 1:25 PM
feld retitled this revision from to bootpd: add support for disabling ARP cache modifications.
feld updated this object.
feld edited the test plan for this revision. (Show Details)
feld added a subscriber: allanjude.
Herald added a reviewer: manpages. · View Herald TranscriptMay 19 2015, 1:25 PM
Herald added a subscriber: imp. · View Herald Transcript
imp accepted this revision.May 19 2015, 4:01 PM
imp added a reviewer: imp.

Other than hating the double indirection through no, not and false, this is a good one.
That is to say, negative options can be hard to follow. But in this case it isn't too
horrible, so I don't object on that basis.

This revision is now accepted and ready to land.May 19 2015, 4:01 PM
wblock added a subscriber: wblock.May 19 2015, 5:50 PM
In D2581#47961, @imp wrote:

Other than hating the double indirection through no, not and false, this is a good one.
That is to say, negative options can be hard to follow. But in this case it isn't too
horrible, so I don't object on that basis.

Changing the name of the variable from noarp to arpmod and inverting the logic would make it a lot easier to read. Other than that, the -a flag usually means "all", possibly not a big deal here.

feld added a comment.May 19 2015, 7:57 PM

After learning of this PR+patch sitting for ~14 years, seeking it out, updating it, pushing into Phabric... the author admitted in a bugzilla comment that he (finally) no longer uses this.

So I guess now it's a matter of "does anyone really care?" I get the concept and why one would want to use this. I personally have no use for it, but maybe someone out there does.

But if not, please close this and we'll move on.

I would like to note that he still has a handful of open PRs with patches that have been left to rot. It sounded to me like he has a lot of other patches but keeps them local and in sync with the latest releases as the effort to submit has not been worth it. That makes me sad.

dan_freebsd.org_obluda.cz accepted this revision.May 20 2015, 9:11 AM
dan_freebsd.org_obluda.cz added a reviewer: dan_freebsd.org_obluda.cz.
dan_freebsd.org_obluda.cz added a subscriber: dan_freebsd.org_obluda.cz.

As I'm author of the original report, I would like to clarify details. The issue addresses by this proposal still exist and the patch solve them. I no longer use BOOTPD for network management (we are using DHCPD instead). In the fact, I'm not sure there is even one actively maintained installation using BOOTPD on the today's world.

In short, I'm no longer personally interested in this patch, but it doesn't change it's effect for others.

I don't care the exact value name nor command line option.

dan_freebsd.org_obluda.cz added a comment.May 20 2015, 9:14 AM

Just one more note - despite I'm not using the patch now, I used it for long time, so it may be considered "verified in production".

Despite not on the current FreeBSD version ...

But BOOTPD source code has not been changed in the past ten years or so.

feld added a comment.Jul 10 2015, 2:56 PM

Does anyone feel this is worthwhile or is there a better chance that bootpd will be removed in the near-ish future?

feld updated this revision to Diff 8695.Sep 12 2015, 7:17 PM
feld edited edge metadata.

rename noarp to arpmod per wblock@ suggestion

This revision now requires review to proceed.Sep 12 2015, 7:17 PM
Herald edited edge metadata. · View Herald TranscriptSep 12 2015, 7:17 PM
feld updated this revision to Diff 8696.Sep 12 2015, 7:20 PM
feld edited edge metadata.

Missed a few noarp -> arpmod renames

Herald edited edge metadata. · View Herald TranscriptSep 12 2015, 7:20 PM
feld updated this revision to Diff 8697.Sep 12 2015, 7:22 PM
feld edited edge metadata.

repeat, sorry for the noise :(

Herald edited edge metadata. · View Herald TranscriptSep 12 2015, 7:22 PM
feld updated this revision to Diff 8698.Sep 12 2015, 7:56 PM
feld edited edge metadata.

Build was successful when I opened this review.
It stopped working (new clang?) due to error:

/usr/src/libexec/bootpd/bootpgw/bootpgw.c -o bootpgw.o
/usr/src/libexec/bootpd/bootpgw/bootpgw.c:662:18: error: use of undeclared identifier 'dst'

inet_ntoa(dst), haddrtoa(ha, len));
          ^

/usr/src/libexec/bootpd/bootpgw/bootpgw.c:663:14: error: use of undeclared identifier 'dst'

setarp(s, &dst, haf, ha, len);

Solve by defining just like in libexec/bootpd/bootpd.c

Herald edited edge metadata. · View Herald TranscriptSep 12 2015, 7:56 PM
feld updated this revision to Diff 8699.Sep 12 2015, 8:00 PM
feld edited edge metadata.

Restore whitespace I clobbered

Herald edited edge metadata. · View Herald TranscriptSep 12 2015, 8:00 PM
dan_freebsd.org_obluda.cz accepted this revision.Sep 12 2015, 8:15 PM
dan_freebsd.org_obluda.cz edited edge metadata.

Look good, thanks.

This revision is now accepted and ready to land.Sep 12 2015, 8:15 PM
emaste added a subscriber: emaste.Nov 16 2018, 8:35 PM

IMO ought to be committed.

dan_freebsd.org_obluda.cz added a comment.Nov 16 2018, 9:12 PM
In D2581#385098, @emaste wrote:

IMO ought to be committed.

I'm still tuned, but 17 years after PR filled I lost most of hope and enthusiasm.

Closed by commit rS348066: bootpd: Add an option to skip modifications to the ARP table. (authored by markj). · Explain WhyMay 21 2019, 9:23 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
libexec/
bootpd/
2 lines
17 lines
bootpgw/
35 lines

Diff 5472

View Options

libexec/bootpd/bootpd.8

Loading...
View Options

libexec/bootpd/bootpd.c

Loading...
View Options

libexec/bootpd/bootpgw/bootpgw.c

Loading...