Close comments which have been implemented

Thanks for the good work Ben.

In D22012#826303, @woodsb02 wrote:

Implement review comments:

• Add dhcpcd as a separate package (not in "runtime" package)
• Allow dhcpcd to be compiled without IPv4 or IPv6 support
• Remove duplicate system directory configuration
• Bump manpage dates
• Simplify rc.conf(5) language describing master mode

how to specifically disable ipv6 ( or ipv4 ) when dhcpcd is in /usr/src?

Some or all of this needs pushing upstream instead of/in addition to local changes. Since the upstream maintainer is subscribed, I haven't made the distinction.

I'd suggest updating this review to be a diff against the copy of dhcpcd now in vendor/ merged into contrib/, so that changes if any to existing dhcpcd files are clear

In D22012#826669, @emaste wrote:

I'd suggest updating this review to be a diff against the copy of dhcpcd now in vendor/ merged into contrib/, so that changes if any to existing dhcpcd files are clear

That would help me greatly bringing any changes requested here upstream.

In D22012#818455, @emaste wrote:

As far as I can tell the objections that were raised over the last few years have all been addressed, and it doesn't seem like much progress has been made on the proposed alternatives. It seems reasonable to me to import dhcpcd initially controlled by a build knob (i.e., WITH_DHCPCD) to allow for further development and experimentation.

This seems reasonable. Just add DHCPCD to DEFAULT_NO and we should be good to go :)

In D22012#826671, @roy_marples.name wrote:

In D22012#826669, @emaste wrote:

I'd suggest updating this review to be a diff against the copy of dhcpcd now in vendor/ merged into contrib/, so that changes if any to existing dhcpcd files are clear

That would help me greatly bringing any changes requested here upstream.

and hopefully it will be easier to review here too :)
Win win

In D22012#826551, @moonlapse81_gmail.com wrote:

how to specifically disable ipv6 ( or ipv4 ) when dhcpcd is in /usr/src?

As per the rest of the FreeBSD src branch - add one of the following to your /etc/src.conf file before running make buildworld:

• WITHOUT_INET=yes
• WITHOUT_INET6=yes

In D22012#826669, @emaste wrote:

I'd suggest updating this review to be a diff against the copy of dhcpcd now in vendor/ merged into contrib/, so that changes if any to existing dhcpcd files are clear

No changes have been made in this review from the copy in the vendor/dhcpcd branch.
The only difference if that rather than being a direct copy/paste, I have used the mechanism built into dhcpcd to load the src into contrib/dhcpcd - this is described in contrib/dhcpcd/README.FREEBSD - extract below for convenience.

The source is imported via a Makefile target rather than by hand.
There is no README.DELETED for this import as it's all automated.

Use "git diff ../vendor/dhcpcd contrib/dhcpcd" to see local modifications.

The program, hook scripts and configuration file are installed by 'sbin/dhcpcd'.

Upgrade notes
-------------

1. Configure
2. Import
3. Copy config.h to sbin/dhcpcd
4. Tailor Makefile in sbin/dhcpcd to import

$ ./configure
$ make import-src DESTDIR=/usr/src/contrib/dhcpcd
$ cp config.h /usr/src/sbin/dhcpcd
$ vi /usr/src/sbin/dhcpcd/Makefile

The following files are only in contrib/dhcpcd:

• hooks/30-hostname
• hooks/50-ypbind
• hooks/dhcpcd-run-hooks
• hooks/dhcpcd-run-hooks.8
• src/dhcpcd-embedded.c
• src/dhcpcd-embedded.h
• src/dhcpcd.8
• src/dhcpcd.conf.5
• README.FREEBSD

The following files are only in vendor/dhcpcd (not brought into contrib/dhcpcd by the "make import-src"):

• .git/*
• compat/crypt/md5.c
• compat/crypt/md5.h
• compat/crypt/sha256.c
• compat/crypt/sha256.h
• compat/arc4random.c
• compat/arc4random.h
• compat/arc4random_uniform.c
• compat/arc4random_uniform.h
• compat/dprintf.c
• compat/dprintf.h
• compat/endian.h
• compat/queue.h
• compat/reallocarray.c
• compat/reallocarray.h
• compat/setproctitle.c
• compat/setproctitle.h
• compat/strlcpy.c
• compat/strlcpy.h
• hooks/30-hostname.in
• hooks/50-dhcpcd-compat
• hooks/50-yp.conf
• hooks/50-ypbind.in
• hooks/Makefile
• hooks/dhcpcd-run-hooks.8.in
• hooks/dhcpcd-run-hooks.in
• src/dev/*
• src/GNUmakefile
• src/Makefile
• src/dev.c
• src/dhcpcd-definitions-small.conf
• src/dhcpcd-definitions.conf
• src/dhcpcd-embedded.c.in
• src/dhcpcd-embedded.h.in
• src/dhcpcd.8.in
• src/dhcpcd.conf.5.in
• src/genembedc
• src/genembedh
• src/if-linux-wext.c
• src/if-linux.c
• src/if-sun.c
• src/privsep-linux.c
• src/privsep-sun.c
• tests/*
• .gitignore
• BUILDING.md
• Makefile
• Makefile.inc
• config-null.mk
• configure
• iconfig.mk

In D22012#826694, @imp wrote:

In D22012#826671, @roy_marples.name wrote:

In D22012#826669, @emaste wrote:

I'd suggest updating this review to be a diff against the copy of dhcpcd now in vendor/ merged into contrib/, so that changes if any to existing dhcpcd files are clear

That would help me greatly bringing any changes requested here upstream.

and hopefully it will be easier to review here too :)
Win win

Given I have not made any changes to the contents of contrib/dhcpcd (other than adding the new README.FreeBSD file), suggest reviewers could exclude contrib/dhcpcd (focus on other changes in the review)?

In D22012#826644, @pauamma wrote:

Some or all of this needs pushing upstream instead of/in addition to local changes. Since the upstream maintainer is subscribed, I haven't made the distinction.

Thanks for the detailed review Pau!

I am trying to keep the contrib/dhcpcd code exactly as per upstream, so rather than fix them here, I have submitted a pull request to upstream on your behalf:
https://github.com/NetworkConfiguration/dhcpcd/pull/123

The only comments of yours I didnt include were:

• Your question about what was meant by IFF_UP - will leave it to @roy_marples.name to clarify wording
• Your question about whether REBOOT was correct - will leave it to @roy_marples.name to clarify wording
• Removing the .Nm from "dhcpcd" at the beginning of the ENVIRONMENT section of the manpage - as I suspect it is either referring to the name of the binary when executed, or is for consistency showing the bold name at the beginning of a new section. @roy_marples.name?

I've addressed the IFF_UP and REBOOT comments with hopefully suitable answers but not marked them as done as I think the asker needs to be satisified and close themselves?

In D22012#826793, @roy_marples.name wrote:

I've addressed the IFF_UP and REBOOT comments with hopefully suitable answers but not marked them as done as I think the asker needs to be satisified and close themselves?

I'm fine with them being marked "done", but I think only the review author (@woodsb02) can do that.

In D22012#826786, @woodsb02 wrote:

In D22012#826644, @pauamma wrote:

Some or all of this needs pushing upstream instead of/in addition to local changes. Since the upstream maintainer is subscribed, I haven't made the distinction.

Thanks for the detailed review Pau!

I am trying to keep the contrib/dhcpcd code exactly as per upstream, so rather than fix them here, I have submitted a pull request to upstream on your behalf:
https://github.com/NetworkConfiguration/dhcpcd/pull/123

Thanks for that. It looks like that was committed upstream meanwhile.

@woodsb02 / everyone: What's the state of this?

I've recently had issues with both dhclient and rtsold and decided to try dhcpcd as it also adds dhcpv6 client support.
Dhcpcd from ports works great and I could put that into our downstream images but I'd like to avoid doing that and then having a version in base a couple months later.

Conversation on the mailing list seems to have stalled and it doesn't seem like there would be an issue as long as dhclient is still available.

In D22012#862131, @freebsd_steffen.cc wrote:

@woodsb02 / everyone: What's the state of this?

I've recently had issues with both dhclient and rtsold and decided to try dhcpcd as it also adds dhcpv6 client support.
Dhcpcd from ports works great and I could put that into our downstream images but I'd like to avoid doing that and then having a version in base a couple months later.

Conversation on the mailing list seems to have stalled and it doesn't seem like there would be an issue as long as dhclient is still available.

I suppose as long as,

A) There is a build knob to enable/disable building this.
B) In addition it has to have its own rc script and that the new rc script and the older dhclient rc script are enabled differently in ifconfig_IF= differently in rc.conf to allow users to select the old and new way. For example, DHCP used for the old dhclient script while NDHCP (or something else) used for this new dhcpd,
C) At some point should it be decided to deprecate the old dhclient DHCP for the dhclient way would be renamed to ODHCP while the NDHCP renamed to DHCP.

In other words we need a plan for how this will be implemented and its future in FreeBSD and how its future will affect the dhclient future in FreeBSD.

If this is to be imported into FreeBSD, sample IPv4 and IPv6 DHCP configurations must be provided.

One related note, the dhclient in the FreeBSD base system no longer has an upstream -- it is OpenBSD's modified version of ISC dhclient, but OpenBSD has subsequently renamed it and reworked it extensively.

Sorry for not providing an update on this sooner.
The summary is I was waiting on the next release of dhcpcd before proceeding with importing it into base.
The reason was a regression related to capsicum was noticed with FreeBSD-14 starting in August 2022 where the privilged process could no longer write to the routing table. This issue isn't occurring on FreeBSD-13, nor FreeBSD-14 from 2021.
@roy_marples.name found and fixed the issue in dhcpcd, but only in what will become dhcpcd-10 (it's too invasive for dhcpcd-9 at this point which is the stable branch).
The dhcpcd-10 has been delayed a bit, since Roy has a few health issues that are his priority right now.
That said, I'm still hoping this can be merged into FreeBSD-14 before the code slush begins on 25th April 2023.

In D22012#862257, @woodsb02 wrote:

Sorry for not providing an update on this sooner.
The summary is I was waiting on the next release of dhcpcd before proceeding with importing it into base.
The reason was a regression related to capsicum was noticed with FreeBSD-14 starting in August 2022 where the privilged process could no longer write to the routing table. This issue isn't occurring on FreeBSD-13, nor FreeBSD-14 from 2021.

Thank you for the clarification! Just to check: the relevant change Is commit @ 30 Aug, right? This looks like a regression, but the commit description looks a bit vague. I wasn't able to find any bug report or message in the ML. Did you / @roy_marples.name reported it? If that's a regression, maybe something can be change on the kernel side, so dhcp 10 is not exactly a blocker.

@roy_marples.name found and fixed the issue in dhcpcd, but only in what will become dhcpcd-10 (it's too invasive for dhcpcd-9 at this point which is the stable branch).
The dhcpcd-10 has been delayed a bit, since Roy has a few health issues that are his priority right now.
That said, I'm still hoping this can be merged into FreeBSD-14 before the code slush begins on 25th April 2023.

In D22012#862327, @melifaro wrote:

Thank you for the clarification! Just to check: the relevant change Is commit @ 30 Aug, right? This looks like a regression, but the commit description looks a bit vague. I wasn't able to find any bug report or message in the ML. Did you / @roy_marples.name reported it? If that's a regression, maybe something can be change on the kernel side, so dhcp 10 is not exactly a blocker.

It is a regression in FreeBSD-14. I updated my VM to the latest FreeBSD-current and the issue is still there.
I didn't report it to any ML as I'm happier with the new code in dhcpcd-10.
In a nutshell it seems like the dhcpcd privileged process seems to be under capsicum control for *some* operations but not others and some in kernel message buffers seem to shrink as writev reports message too long.
The same code works fine on FreeBSD-13.

Happy to discuss this elsewhere?

In D22012#862511, @roy_marples.name wrote:

In D22012#862327, @melifaro wrote:

Thank you for the clarification! Just to check: the relevant change Is commit @ 30 Aug, right? This looks like a regression, but the commit description looks a bit vague. I wasn't able to find any bug report or message in the ML. Did you / @roy_marples.name reported it? If that's a regression, maybe something can be change on the kernel side, so dhcp 10 is not exactly a blocker.

It is a regression in FreeBSD-14. I updated my VM to the latest FreeBSD-current and the issue is still there.
I didn't report it to any ML as I'm happier with the new code in dhcpcd-10.
In a nutshell it seems like the dhcpcd privileged process seems to be under capsicum control for *some* operations but not others and some in kernel message buffers seem to shrink as writev reports message too long.
The same code works fine on FreeBSD-13.

Happy to discuss this elsewhere?

Sure! I guess the best-suited platform for that would be our bugzilla - as I guess we need to get sort of a repro (or at least more detailed description) and go from here. Do you mind submitting something?

dhcpcd-10 will be released in the next few week or so. Following that, I intend to update this diff to target importing dhcpcd into base ahead of the FreeBSD-14 code slush.

In D22012#894877, @woodsb02 wrote:

dhcpcd-10 will be released in the next few week or so. Following that, I intend to update this diff to target importing dhcpcd into base ahead of the FreeBSD-14 code slush.

Just a reminder that dhcpcd-10 has been released and is ready for import into FreeBSD-14.
Well, from my perspective anyway.

@woodsb02: Are you working on a newer patch for dhcpcd-10 for inclusion into FreeBSD14?
Would really be great to have. If this needs any shepherding from core, I'll see what I can do.

Is there something we can do to make dhcpcd part of 14.0-RELEASE besides testing this patch?

I don't have any IPv6-enabled network, but the test plan above looks simple enough for me to test with eduroam and one or two other environments.

Re: the schedule at https://www.freebsd.org/releases/14.0R/, I can probably find time to test before the first beta.

This patch fails to apply to CURRENT. I am using dhcpcd-10 since it was released (and used previous versions) for DHCP, SLAAC and DHCPv6, all on the same network. That's the closest to testing I can get in current state. If this patch is updated I would love to test it. Of course, if updating patch is required, I would love to do it, but at this point I'm not sure I know how.

Would be good to get this in main before stable/14 branches. Should we look for someone else to do the work if @woodsb02 doesnt have the cycles right now?

I just pushed out dhcpcd-10.0.2 which fixes some privsep issues.

dhcpcd-10.0.3 just released which also fixes some important stuff.

Is there anything I can do to help accelerate this ticket?

I plan on taking this further during the holidays.

This clearly has quite a bit of attention!
Is it PkgBase-friendly?
Are there any blockers other than the usual re-basing as time moves on?
If a vendor-branch import, how difficult is the maintenance burden?

In D22012#1018620, @editor_callfortesting.org wrote:

This clearly has quite a bit of attention!
Is it PkgBase-friendly?

@freebsd_igalic.co Is that something you have (time for) an opinion on?