dhcpv4: cleanups, use iovec by Alphix · Pull Request #278 · openwrt/odhcpd

Alphix · 2025-10-13T16:28:34Z

This is the first (and biggest) PR I have cooking for dhcpv4.c. There's a lot of changes, but what it boils down to is getting rid of dhcpv4_put() (which copies a lot of data around on the stack before sending it out, which I consider to be a poor choice...I think DHCPv4 has definitely gotten less love than its DHCPv6 counterpart).

In the process, the monster function that deals with incoming DHCPv4 requests is substantially reworked so that it has two distinct phases, parse incoming options (one switch statement), determine whether a reply is due, then preparing the outgoing options (another switch statement).

This will make it a lot easier to to hack on dhcpv4.c in the future (compare the files in the pre and post PR versions and I think you'll agree).

Right now, this is based on top of the other PR which reworks syslog messages (PR #273), so that needs attention first, but I'm posting this now already so that @systemcrash and @Noltari can have a look :)

There's another follow-up PR or two after this one, but that can wait 😄

systemcrash · 2025-10-14T00:06:52Z

The change set looks great in general. Good use of iovec. Looking forward to a more maintainable interface.

Will be easier to review once the log stuff goes in.

systemcrash · 2025-10-18T18:39:22Z

ping @Alphix for rebase

Alphix · 2025-10-19T05:22:13Z

ping @Alphix for rebase

🫡

Alphix · 2025-10-19T16:26:39Z

@Noltari when you have time, please have a look :)
@systemcrash should be easier to review now

systemcrash · 2025-10-19T17:04:55Z

src/dhcpv4.c

-		   MSG_DONTWAIT, (struct sockaddr*)&dest, sizeof(dest)) < 0)
-		error("Failed to send %s to %s - %s: %m", dhcpv4_msg_to_string(msg),
+	if (dhcpv4_send_reply(iov, ARRAY_SIZE(iov), (struct sockaddr *)&dest, sizeof(dest),
+			      &a->iface->dhcpv4_event.uloop.fd) < 0)


nitpick: tabs

I think that's the same thing we've discussed earlier...vim and kernel coding style and all that...it lines up correctly if you have tabs = 8 characters :) I guess you want it to be three plain tabs?

src/dhcpv4.c

systemcrash · 2025-10-19T17:19:33Z

@Noltari when you have time, please have a look :) @systemcrash should be easier to review now

Yeah - good. Can't fault anything.

Noltari

LGTM, @systemcrash any objections?

systemcrash · 2025-10-20T10:38:07Z

LGTM

dhcpv4.c currently builds DHCPv4 messages by lots of repeated calls to the dhcpv4_put() method, which basically does a whole lot of copies to the stack. The following set of patches convert dhcpv4.c over to use iovecs, which avoids almost all of the stack allocations and copies. The first step is to lay the groundwork by changing dhcpv4_send_reply() and the corresponding dhcpv6_4o6_send_reply() in dhcpv6.c so that they both take an iovec instead of a plain buffer. Also introduce the first (very very simple) iovec for dhcpv4_handle_msg() which just contains the whole packet, plus the end marker as a separate vector element. It will be filled out further in subsequent patches. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Most notably, neither hostname, nor reqopts actually need to be copied from req, they are used in a read-only fashion throughout the rest of the function (and any functions it calls). Also switch some complicated if-else constructs over to switch-case. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Make it clearer which variables relate to the request, and which ones relate to the reply that we're constructing. Also fix the missing endianness conversion for req_leasetime (not so severe though, a bogus leasetime value will be overridden with limits that are set per-lease or per-interface). Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Move some gnarly conditionals into a switch in preparation of later patches. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Ok, this is a bit weird, dhcpv4_handle_msg() passes reqopts to dhcpv4_lease(), which stashes a copy of the reqopts in the struct dhcp_assignment, and then dhcpv4_handle_msg() uses the stashed copy instead. The only reason a copy is made seems to be so that the requested options can later be reported via ubus (in "ubus call dhcp ipv4leases"), but it is hard to see a use-case for that (the DHCP server has already replied to the client by the time the lease is available over ubus) and we don't do anything similar for DHCPv6, so remove the extra copying. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

dhcpv4_handle_msg() is already over 300 lines of code, break out the destination handling into a separate function to make the former function a bit more manageable in size. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

It's only used once, and there's no lack of variables to keep track of in dhcpv4_handle_msg(). Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Sort the variables into request and response variables for clarity. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Start converting options over to an iovec based solution. Also change the name of "type" in struct dhcpv4_option, both the DHCPv4 and DHCPv6 RFCs call these "option codes", not types. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Note that the previous code would send out several NTP options if the client requested the option more than once, and would also send out zero-sized options (against RFC2132, §8.3). Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Another simple conversion to use iovec. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Add a simple array to record the options we want to send back. This is just a placeholder in preparation for later patches. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Move some more options over to use an iovec. Note that the FIXME will be addressed in a later patch. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

More simple conversions. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Some more options converted. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

And some more options converted. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

That's the last bunch of options :) Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Do some cleanups, remove some magic numbers, make sure structs are defined with the members in their actual order and without missing members. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Fix the FIXME added earlier by calculating the length of the iovec package and adding padding as necessary to comply with the BOOTP specification (which defines the minimum length of a message as 300 bytes). Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Do the same iovec conversion for dhcpv4_fr_send(), and remove dhcpv4_put() now that the last user is gone. Yay. Note that there was a bug in dhcpv4_fr_send() in that it would first perform: md5_hash(&fr_msg, sizeof(fr_msg), &md5); And later: sendto(/* fd */, &fr_msg, PACKET_SIZE(&fr_msg, cursor), ...) But PACKET_SIZE is much smaller than sizeof(fr_msg), meaning that the hash is not computed for the actual packet that gets sent. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

The options padding can now be removed. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Instead of copying around the same data on the stack, keep two arrays with mandatory and requested options, then loop over both of them. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

And make it clearer that the return value from dhcpv4_lease isn't used. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Noltari · 2025-10-21T17:16:23Z

Merged, thanks @Alphix!

Alphix · 2025-10-21T17:23:07Z

Yay!

testuser7 · 2026-01-18T11:40:37Z

src/dhcpv4.c

-		debug("Sent %s to %s - %s",
-		      dhcpv4_msg_to_string(msg),
-		      dest.sin_addr.s_addr == INADDR_BROADCAST ?
+		error("Sent %s to %s - %s",


@Alphix was this change from debug to error intentional?

Hmm. Trivial to fix.

Alphix mentioned this pull request Oct 15, 2025

dhcpv6: message type check refactor #279

Merged

Alphix force-pushed the dhcpv4_use_iovec branch from 0b4bb3b to df32d6e Compare October 19, 2025 05:17

Alphix force-pushed the dhcpv4_use_iovec branch from df32d6e to 92ae092 Compare October 19, 2025 16:39

systemcrash reviewed Oct 19, 2025

View reviewed changes

src/dhcpv4.c Show resolved Hide resolved

Noltari approved these changes Oct 20, 2025

View reviewed changes

Alphix added 18 commits October 21, 2025 19:03

dhcpv4: more refactoring of dhcpv4_handle_msg()

b1be398

Move some gnarly conditionals into a switch in preparation of later patches. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

dhcpv4: remove one more variable from dhcpv4_handle_msg()

18c1b02

It's only used once, and there's no lack of variables to keep track of in dhcpv4_handle_msg(). Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

dhcpv4: reorder some more variables in dhcpv4_handle_msg()

2f97bf0

Sort the variables into request and response variables for clarity. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

dhcpv4: use iovec for DNR

0533eae

Another simple conversion to use iovec. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

dhcpv4: use iovec for leasetime/renew/rebind

5bafc17

Move some more options over to use an iovec. Note that the FIXME will be addressed in a later patch. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

dhcpv4: use iovec for netmask/hostname/broadcast

578a928

More simple conversions. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

dhcpv4: use iovec for DNS search and MTU

b81cfaa

Some more options converted. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

dhcpv4: use iovec for router and DNS server

bd353e8

And some more options converted. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

dhcpv4: use iovec for forcerenew opts

e24a371

That's the last bunch of options :) Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Alphix added 5 commits October 21, 2025 19:05

dhcpv4: shrink struct dhcpv4_message

d63fa3c

The options padding can now be removed. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

dhcpv4: bail earlier on release/decline

54b9e72

And make it clearer that the return value from dhcpv4_lease isn't used. Signed-off-by: David Härdeman <david@hardeman.nu> Link: openwrt#278 Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Noltari force-pushed the dhcpv4_use_iovec branch from 92ae092 to 54b9e72 Compare October 21, 2025 17:06

openwrt-bot merged commit 54b9e72 into openwrt:master Oct 21, 2025
5 checks passed

Alphix deleted the dhcpv4_use_iovec branch October 21, 2025 17:23

testuser7 reviewed Jan 18, 2026

View reviewed changes

Conversation

Alphix commented Oct 13, 2025

Uh oh!

systemcrash commented Oct 14, 2025

Uh oh!

systemcrash commented Oct 18, 2025

Uh oh!

Alphix commented Oct 19, 2025

Uh oh!

Alphix commented Oct 19, 2025

Uh oh!

systemcrash Oct 19, 2025

Choose a reason for hiding this comment

Uh oh!

Alphix Oct 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

systemcrash commented Oct 19, 2025

Uh oh!

Noltari left a comment

Choose a reason for hiding this comment

Uh oh!

systemcrash commented Oct 20, 2025

Uh oh!

Uh oh!

Noltari commented Oct 21, 2025

Uh oh!

Alphix commented Oct 21, 2025

Uh oh!

testuser7 Jan 18, 2026

Choose a reason for hiding this comment

Uh oh!

systemcrash Jan 18, 2026

Choose a reason for hiding this comment

Uh oh!

systemcrash Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments