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.
Optimally we want stateless, pure code. Obviously, NMDhcpClient needs to
keep state to know what it's doing. However, we should well encapsulate
the state inside NMDhcpClient, and only accept events/notifications that
mutate the internal state according to certain rules.
Having a function public set_state(self, new_state) means that other
components (subclasses of NMDhcpClient) can directly mangle the state.
That means, you no longer need to only reason about the internal state
of NMDhcpClient (and the events/notifications/state-changes that it
implements). You also need to reason that other components take part of
maintaining that internal state.
Rename nm_dhcp_client_set_state() to nm_dhcp_client_notify(). Also, add
a new enum NMDhcpClientEventType with notification/event types.
In practice, this is only renaming. But naming is important, because it
suggests the reader how to think about the code.
The "noop" state is almost unused, however, nm_dhcp_set_state()
has a check "if (new_state >= NM_DHCP_STATE_TIMEOUT)", so the order
of the NOOP state matters.
Fix that by reordering.
Also, just return right away from NOOP.
NMDhcpState is very tied to events from dhclient. But most of these
states we don't care about, and NMDhcpClient definitely should abstract
and hide them.
We should repurpose NMDhcpState to simpler state. For that, first drop
the state from nm_dhcp_client_handle_event().
This is only the first step (which arguably makes the code more
complicated, because reason_to_state() gets spread out and the logic
happens more than once). That will be addressed next.
- return early to avoid nested block.
- use NM_STR_HAS_PREFIX() over g_str_has_prefix(), because that
can be inlined and only accepts a C literal as prefix argument.
