[th/dhcp-internal-mulitple-routers] #265
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lkundrak
force-pushed
the
th/dhcp-internal-mulitple-routers
branch
from
3ca070e
to
1d6c01d
Compare
lkundrak
force-pushed
the
th/dhcp-internal-mulitple-routers
branch
4 times, most recently
from
2493472
to
b29d2bd
Compare
thom311
added
the
needs-work
lkundrak
approved these changes
Jan 21, 2019
| @@ -41,7 +41,6 @@ struct sd_dhcp_lease { | |||
| /* each 0 if unset */ | |||
There was a problem hiding this comment.
Systemd-pull-request: #11208
Please include a full URL here
thom311
force-pushed
the
th/dhcp-internal-mulitple-routers
branch
from
b29d2bd
to
e8a2253
Compare
|
OK, the systemd pull request got merged. I incorporated the patches that were merged to systemd upstream and rebased the branch again. Please review. Note that I still don't refer directly to the pull-request (via github URL), because that causes a link-back and ping. I will adjust the URLs before merging. |
thom311
removed
the
needs-work
…onfig()
... and lease_to_ip6_config().
- Handle reasons that render the lease invalid first, before logging
anything. This way, upon invalid lease we don't have partially logged
about the lease.
- prefer logging one line for options that contain multiple values, for
example for search domains.
- reorder statements to consistently log first before calling add_option().
- prefer
g_string_append (nm_gstring_add_space_delimiter (str), ...
over
g_string_append_printf (str, "%s%s", str->len ? " " : "", ...
- use @addr_str buffer directly, instead of assigning to another
temporary variable.
…eserialize_in_addrs()
Imported from systemd:
deserialize_in_addrs() allocates the buffer before trying to parse
the IP address. Since a parsing error is silently ignored, the returned
size might be zero. In such a case we shouldn't return any buffer.
Anyway, there was no leak, because there are only two callers like
r = deserialize_in_addrs(&lease->dns, dns);
which both keep the unused buffer and later release it.
Note that deserialize_in_addrs() doesn't free the pointer before
reassigning the new output. The caller must take care to to pass
"ret" with an allocated buffer that would be leaked when returning
the result.
systemd/systemd@c24b682
…HCP library
Imported from systemd:
The Router DHCP option may contain a list of one or more
routers ([1]). Extend the API of sd_dhcp_lease to return a
list instead of only the first.
Note that networkd still only uses the first router (if present).
Aside from extending the internal API of the DHCP client, there
is almost no change in behavior. The only visible difference in
behavior is that the "ROUTER" variable in the lease file is now a
list of addresses.
Note how RFC 2132 does not define certain IP addresses as invalid for the
router option. Still, previously sd_dhcp_lease_get_router() would never
return a "0.0.0.0" address. In fact, the previous API could not
differenciate whether no router option was present, whether it
was invalid, or whether its first router was "0.0.0.0". No longer let
the DHCP client library impose additional restrictions that are not
part of RFC. Instead, the caller should handle this. The patch does
that, and networkd only consideres the first router entry if it is not
"0.0.0.0".
[1] https://tools.ietf.org/html/rfc2132#section-3.5
This also required adjusting "src/dhcp/nm-dhcp-systemd.c" due to the
changed internal API.
systemd/systemd@f886239
…client
Imported from systemd:
The DHCP client should not pre-filter addresses beyond what RFC
requires. If a client's user (like networkd) wishes to skip/filter
certain addresses, it's their responsibility.
The point of this is that the DHCP library does not hide/abstract
information that might be relevant for certain users. For example,
NetworkManager exposes DHCP options in its API. When doing that, the
options should be close to the actual lease.
This is related to commit d9ec2e632df4905201facf76d6a205edc952116a
(dhcp4: filter bogus DNS/NTP server addresses silently).
systemd/systemd@072320e
Imported from systemd:
inet_ntop() is not documented to be thread-safe, so it should not
be used in the DHCP library. Arguably, glibc uses a thread local
buffer, so indeed there is no problem with a suitable libc. Anyway,
just avoid it.
systemd/systemd@189255d
- regarding the DHCP options, we should not suppress them. If the lease contains such bogus(?) addresses, we still want to expose them on D-Bus without modification. - regrading using the DNS server, ignore localhost addresses like done for systemd-networkd ([1], [2]). Until recently, the DHCP library would internally suppress such addresses ([3]). That is no longer the case, and we should handle them specially. [1] Systemd-pull-requst #4524 [2] systemd/systemd@d9ec2e6 [3] Systemd-pull-request #11208
lkundrak
force-pushed
the
th/dhcp-internal-mulitple-routers
branch
from
e8a2253
to
07badc7
Compare
|
LGTM |
|
Thanks. Merged to master |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to rh#1634657 and #256 .
Support multiple gateways in the Router option.
At the moment, this is still pending on an upstream systemd pull request.