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.

Sign up for GitHub

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

Jump to bottom

network: several cleanups and fixes for DHCPv4 client #28508

Merged
merged 12 commits into from Jul 29, 2023
Merged

network: several cleanups and fixes for DHCPv4 client #28508

merged 12 commits into from Jul 29, 2023

Conversation

yuwata
Copy link
Member

@yuwata yuwata commented Jul 24, 2023 •
edited by github-actions bot

Fixes #28495.

@yuwata yuwata added this to the v255 milestone Jul 24, 2023
@github-actions github-actions bot added util-lib tests please-review PR is ready for (re-)review by a maintainer labels Jul 24, 2023
@github-actions
Copy link

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

@yuwata yuwata mentioned this pull request Jul 24, 2023
Closed
keszybz
keszybz requested changes Jul 27, 2023
View reviewed changes
Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

I looked at the code… I can't say I could follow the details of route selection. But since there are tests that carefully check the result, I hope it's all OK. A few comments and naming and spelling and such.

src/network/networkd-dhcp4.c Outdated Show resolved Hide resolved
src/network/networkd-dhcp4.c Outdated Show resolved Hide resolved
src/network/networkd-dhcp4.c Show resolved Hide resolved
@keszybz keszybz added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Jul 27, 2023
@keszybz
Copy link
Member

keszybz commented Jul 27, 2023

I set reviewed/needs-rework because I think somebody else should take a look if there's another version.

yuwata added 12 commits July 28, 2023 05:15
@yuwata
in-addr-util: introduce in_addr_prefix_covers_full() and friends
45e7609
@yuwata
sd-dhcp: introduce sd_dhcp_lease_get_prefix()
33acdc5
@yuwata
network/dhcp4: use sd_dhcp_lease_get_prefix()
2faf2e2
@yuwata
network/dhcp4: honor received broadcast address
a49cf37
@yuwata
network/dhcp4: use FOREACH_ARRAY() macro
1953bcd
@yuwata
network/dhcp4: drop unnecessary assignment
5e6dad1 
It will be set in dhcp4_request_route_auto().
@yuwata
network/dhcp4: introduce dhcp4_get_classless_static_or_static_routes(…
d52c5a7 
…) helper

No functional changes, just refactoring and preparation for later
commits.
@yuwata
network/dhcp4: introduce dhcp4_get_router() helper function
15aa7a5 
Previously, we use the first router address, and if it is null, we
ignore the address. Now, we use the first non-null address. That is, if
the first router address is null, but the second is not, then we use the
second one.

That should not cause functional change in most cases, except for the case
when a DHCP server provides such spurious reply.

This is mostly for refactoring and preparation for later commits.
@yuwata
network/dhcp4: introduce dhcp4_prefix_covers() helper function
0bc8b6e 
No functional change, just refactoring.
@yuwata
network/dhcp4: always find suitable gateway for destination address
0ce86f5 
And if not found, refuse to configure the route.

If a DHCP server provides classless static or static routes, then we
should use the gateway for accessing a node in the range specified in
the route. E.g. if a DHCP server provides the default gateway is
192.168.0.1, and classless static route for 8.0.0.0/8 with gateway
192.168.0.2, then we should access 8.8.8.8 through 192.168.0.2 rather
than 192.168.0.1, but should use 192.168.0.1 for 9.9.9.9.

Fixes systemd#28358.
@yuwata
network/dhcp4: always honor specified gateway address
4001653 
Follow-up for 77451f6.

Now, gateway for routes to DNS or NTP servers should be correctly picked,
hence it is not necessary to adjust the gateway address in
dhcp4_request_route_auto() again.

Also, similar for classless static routes, let's always honor
gateway address specified in (non-classless) static routes.
@yuwata
network/dhcp4: drop unused logic of finding default gateway
a56347d
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jul 27, 2023
@yuwata
Copy link
Member Author

yuwata commented Jul 27, 2023

@keszybz Thank you for the review.

  • renamed function,
  • added more comments in the code,
  • extended commit messages.

PTAL.

@bluca bluca merged commit fc3fe92 into systemd:main Jul 29, 2023
47 of 48 checks passed
@github-actions github-actions bot removed the please-review PR is ready for (re-)review by a maintainer label Jul 29, 2023
@yuwata yuwata deleted the network-next-dhcp4 branch July 29, 2023 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keszybz keszybz keszybz requested changes

@bluca bluca bluca approved these changes

Assignees
No one assigned
Labels
dhcp network tests util-lib
Milestone
v255
3 participants
@yuwata @keszybz @bluca