This was working for internal plugin in the past, but broken by l3cfg
rework with 1.36. Re-add it. Not it also works with dhclient. For other
plugins, it's not really working, because we can't decline.
Now NMDhcpClient does ACD (using NML3Cfg) and abstracts that from
the caller (NMDevice).
It is complicated. Because there is state involved, meaning, we need
to remember the current state for ACD and react on and handle a
multitude of events. Getting this right, is non-trivial.
What we want is that if ACD fails, we decline the lease (and don't use
it).
https://bugzilla.redhat.com/show_bug.cgi?id=1713380
dhclient itself doesn't do ACD. However, it expects the dhclient-script
to exit with non-zero status, which causes dhclient to send a DECLINE.
`man dhclient-script`:
BOUND:
Before actually configuring the address, dhclient-script should
somehow ARP for it and exit with a nonzero status if it receives a
reply. In this case, the client will send a DHCPDECLINE message to
the server and acquire a different address. This may also be done in
the RENEW, REBIND, or REBOOT states, but is not required, and indeed may
not be desirable.
See also Fedora's dhclient-script ([1]).
https://gitlab.isc.org/isc-projects/dhcp/-/issues/67#note_9722633226f2d76/client/dhclient.c (L1652)
[1] a8f6fd046f/f/dhclient-script (_878)https://bugzilla.redhat.com/show_bug.cgi?id=1713380
- assign the result of NM_DHCP_CLIENT_GET_CLASS() to a local variable.
It feels nicer to only call the macro once. Of course, the macro
expands to plain pointer dereferences, so there is little difference
in terms of executed code.
- handle the default case with no virtual function first.
It's pretty pointless to log
<trace> [1653389116.6288] dhcp4 (br0): client event 7
<debug> [1653389116.6288] dhcp4 (br0): received OFFER of 192.168.121.110 from 192.168.121.1
where the obscure event #7 is only telling you that we are going
to log something. Handle logging events first.
In general, drop the "client event %d" message and make sure that all
code paths log something (useful), so we can see in the log that the
event was reached.
When we accept/decline a lease, then that only works if we are in state
GRANTED. n-dhcp4 API also requires us, to provide the exact lease, that
we were announced earlier.
As such, we need to make sure that we don't accept/decline in the wrong
state. That means, to keep track of what we are doing more carefully.
The functions _dhcp_client_accept()/_dhcp_client_decline() now take
a l3cd argument, the one that we announced earlier. And we check that it
still matches.
They are no longer used from outside, NMDhcpClient fully handles this.
Make them static and internal.
Also, decline is currently unused. It will be used soon, with ACD
support.
Previously, during decline we would clear probe->current_lease,
however leave the state at GRANTED.
That is a wrong state, and can easily lead to a crash later.
For example, on the next timeout we will end up at
n_dhcp4_client_dispatch_timer(), then current-lease gets
accessed unconditionally:
case N_DHCP4_CLIENT_PROBE_STATE_GRANTED:
if (ns_now >= probe->current_lease->lifetime) {
Instead, return to INIT state and schedule a timer. As suggested
by RFC 2131, section 3.1, 5) ([1]).
[1] https://datatracker.ietf.org/doc/html/rfc2131#section-3.1
The lease list and the probe's state are strongly related. That is
evidenced by the fact that sometimes we check the state and then
access probe->current_lease without further checking.
The code in "n-dhcp4-c-probe.c" (select_lease, accept, decline) already
changes and maintains the state, it should also maintain the lease list.
Move the code.
The caller is supposed to call accept/decline/select with the lease that
was just announced. Calling it in the wrong state or with the wrong
lease is a user error.
Return an error when called in the wrong state, so that the user
notices they did something wrong.
The same check is also for nettools' n-dhcp4 client. It's useful to
being able to rely on certain things, like that an DHCPv4 lease always
has exactly one address (not equal to 0.0.0.0).
The l3_cfg_notify_cb() handler is used for different purposes, and
different events will be considered.
Usually a switch statement is very nice for enums, especially if all
enum values should be handled (because the compiler can warn about
unhandled cases). In this case, not all events are supposed to be
handled. At this point, it seems nicer to just use an if block. It
better composes.
The compiler should be able to optimize both variants to the same
result. In any case, checking some integers for equality is in any case
going to be efficient.
I think the previous was technically correct in any case too.
Still change it, because I feel with union and struct initialization,
we should always explicitly pick one union member that we fully
initialize.
Of course, blocking and synchronous code is much simpler. But it's also
fundamentally wrong to block while we talk to systemd-hostnamed.
Refactor to use async operations.
Introduction of a new setting ipv4.link-local, which enables
link-local IP addresses concurrently with other IP address assignment
implementations such as dhcp or manually.
No way is implemented to obtain a link-local address as a fallback when
dhcp does not respond (as dhcpd does, for example). This could be be
added later.
To maintain backward compatibility with ipv4.method ipv4.link-local has
lower priority than ipv4.method. This results in:
* method=link-local overrules link-local=disabled
* method=disabled overrules link-local=enabled
Furthermore, link-local=auto means that method defines whether
link-local is enabled or disabled:
* method=link-local --> link-local=enabled
* else --> link-local=disabled
The upside is, that this implementation requires no normalization.
Normalization is confusing to implement, because to get it really
right, we probably should support normalizing link-local based on
method, but also vice versa. And since the method affects how other
properties validate/normalize, it's hard to normalize that one, so that
the result makes sense. Normalization is also often not great to the
user, because it basically means to modify the profile based on other
settings.
The downside is that the auto flag becomes API and exists because
we need backward compatibility with ipv4.method.
We would never add this flag, if we would redesign "ipv4.method"
(by replacing by per-method-specific settings).
Defining a default setting for ipv4.link-local in the global
configuration is also supported.
The default setting for the new property can be "default", since old
users upgrading to a new version that supports ipv4.link-local will not
have configured the global default in NetworkManager.conf. Therefore,
they will always use the expected "auto" default unless they change
their configuration.
Co-Authored-By: Thomas Haller <thaller@redhat.com>
During the deactivation of ovs interfaces, ovsdb receives the command to
remove the interface but for OVS system ports the device won't
disappear.
When reconnecting, ovsdb will update first the status and it will notice
that the OVS system interface was removed and it will set the status as
DEACTIVATING. This is incorrect if the status is already DEACTIVATING,
DISCONNECTED, UNMANAGED or UNAVAILABLE because it will block the
activation of the interface.
https://bugzilla.redhat.com/show_bug.cgi?id=2080236
On m68k we get a static assertion, that NMPlatformIP4Address.address
is not at the same offset as NMPlatformIPAddress.address_ptr.
On most architectures, the bitfields fits in a gap between the fields,
but not on m68k, where integers are 2-byte aligned.
On m68k, integers are 2-byte aligned. Hence the assertion was wrong.
What we really want to check, is that NMIPAddr has not a smaller
alignment than in_addr_t and similar.
While at it, also assert the alignment for NMEtherAddr.
We want to assert that our alignment-guarantees do not exceed the
guarantees of the system-linker or system-allocator on the target
platform. Hence, we check against max_align_t. This is a lower bound,
but not the exact check we actually want. And as it turns out, on m64k
it is too low. Add a static check against 4-byte alignment for m64k as
a workaround.
Reported-by: Michael Biebl
Signed-off-by: David Rheinsberg <david.rheinsberg@gmail.com>
https://github.com/c-util/c-rbtree/issues/9eb778d3969
As almost always, there is a point in keeping IPv4 and IPv6 implementations
similar. Behave different where there is an actual difference, at the bottom
of the stack.
Technically, g_warn_if_reached() may not be an assertion, according to
glib. However, there is G_DEBUG=fatal-warnings and we want to run with
that.
So this is an assertion to us. Also, logging to stderr/stdout is not a
useful thing to the daemon. Don't do this. Especially, since it depends
on user provided (untrusted) input.
