On python2 the following error is raised:
`LookupError: unknown encoding: unicode`
Seems like `unicode` is a correct encoding in Python 3 but not 2.
Fix:
1. Change encoding to `utf-8`
2. Pass output path string instead of opening file and passing
opened file object. Python2 and 3 might need different file
modes, passing just path lets ElementTree select appropriate
file mode.
Fixes: f00e90923c ('tools: Use ElementTree to write XML in generate-docs-nm-settings-docs-gir.py')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1249
Currently, for all tests we have python3 installed. So effectively,
even on CentOS 7, we would test the build with python3 only.
The package build on CentOS7/epel7 however uses python2. This happens
for example for our copr builds.
Also test that configuration in gitlab-ci.
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.
It's just convenient to have some tools around, not only
for testing, but also for (some limited) development.
In particular, because we bind-mount .vimrc inside the container, and
if I use vim, black/clang-format is just one key binding away.
You can of course just clone NetworkManager repository and start hacking
as you like. However, there are a few things like git-notest which are
interesting to setup.
Add a script to do this.
The script is supposed to be idempotent and do nothing, unless
necessary. By default it also only prints what it would do.
Instead of manually writting XML line by line.
Quoting is automatic.
Makes it much easier to modify. (just add new elements)
Generated XML not indented at the moment.
* Create main() function and put its execution under
__name__ == '__main__' guard.
* Only one module import per line
* Use required=True to check if necessary arguments have
been passed.
* Remove usage() as ArgumentParser handles that already
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.
NetworkManager does not support by default legacy ifcfg configuration
files anymore, this support is now provided in a separate package
(https://fedoramagazine.org/converting-networkmanager-from-ifcfg-to-keyfiles/).
ifcfg directory (/etc/sysconfig/network-scripts/) should always be present,
regardless of NetworkManager support for network scripts. This change makes the
directory always present, not only when the recently splitted ifcfg subpackage
is installed, and also make it persistent after the package removal.
Fixes: 50a6627fd7 ('rpm: split ifcfg-rh settings plugin into a separate package')
