It seems slightly nicer not to leave a dangling pointer at the
end of the iteration. Then you could do something like
nm_dedup_multi_iter_init(&iter, head_entry);
while (nm_dedup_multi_iter_next(&iter)) {
if (some_condition())
break;
}
if (!iter.current)
printf("iterated to the end\n");
As nm_dedup_multi_iter_next() and nm_dedup_multi_iter_init() are inline
functions, the compiler should even be able to see that the initial
setting becomes unnecessary (the field will be initialized by the
first nm_dedup_multi_iter_next()). Likewise, the final clearing
of the field might also be optimized away at the end of the iteration
(if, as in the common case, the iterator is not accessed afterwards).
Depending on sizeof(policy) to be sizeof(NULL) is not a good check
whether the macro argument may be NULL. That is, because the size
of the policy array might accidentally be the same as the size of
a pointer. Use _Generic() instead.
- make nla_attr_minlen[] and array of uint8_t. That is large enough for
all values we have.
- don't handle NLA_UNSPEC specially. nla_attr_minlen[NLA_UNSPEC] returns
zero just fine.
NetworkManager handles "add" and "move" actions the same way, by
tracking the "struct udev_device" instance.
Still, this means that also for move events, we need the right
attributes set.
See-also: https://github.com/openshift/sriov-network-operator/issues/414
The user of NMDhcpClient is supposed to call "nm_dhcp_client_stop()"
on the instance, also because it is a ref-counted GObject type. So we
wouldn't want to rely on the last unref to stop DHCP.
Still, in case that was not done, the code somehow made the assumption
it made any sense to possibly not stop the DHCP client. For the internal
client, there is of course nothing left after destroying the
NMDhcpClient instance, but what about dhclient? I don't think we should
ever leave dhclient running unsupervised. Even during restart of the
service, we need to stop it first, and restart it afterwards.
When we quit NetworkManager, we may want to leave the interface up and
configured. For that, we may need to take care that "nm_dhcp_client_stop()"
does not destory the IP configuration of the intrerface. But I don't
think that is a problem. What we still want to do, is kill the dhclient
instance.
NMDhcpClient is not supposed to be ever restarted. It starts when an
instance gets created, and it stops with "nm_dhcp_client_stop()". Hence,
the simple "is_stopped" flag is fine to prevent that multiple stop calls
are harmful.
NMDhcpManager was tracking DHCP clients. During start, it would check
whether an instance for the same ifindex is running, and stop it.
That seems unnecessary and wrong. Clearly, we cannot have multiple users
(like two `NMDevice`s) run DHCP on the same interface. But its up to
them to coordinate that. They also cannot configure IP addresses at the
same interface, and if they do, then there is a big problem already.
This comes from commit 1806235049 ('dhcp: convert dhcp backends to
classes'). Maybe back then there was also the idea that NetworkManager
could quit and leave dhclient running. That idea is also flawed. When
NetworkManager stops, it leaves the interface (possibly) up, so that
restart works without disruption. That does not mean that the DHCP
client needs to keep running. What works is to restart NetworkManager in
a timely manner, then NetworkManager will start a new DHCP client after
restart. What does not work is stop NetworkManager, do nothing (like
taking over the interface by running your own manager) and expect that
DHCP keeps working indefinitely. And of course, with the internal client
this cannot possibly work either. Don't stop NetworkManager for good, if
you expect NetworkManager to run DHCP on an interface.
A different things is that when NetworkManager crashes, that after
restart it kills the left over dhclient instances. That may require a
solution, for example systemd killing all processes or checking for
left-over PID files and kill the processes. But what was implemented in
NMDhcpManager was not a solution for that.
As such, it's not clear what conflicting instance we want to kill, or
why NMDhcpManager should even track NMDhcpClient instances.
NMDhcpClient communicates events via GObject signals. GObject signals in
principle could have several subscribers. In practice, a NMDhcpClient
instance has only one subscriber, because it was constructed with
certain parameters, so it's unlikely to be shared.
That one subscriber, always needs to subscribe to all signals
("state-changed" and "prefix-delegated"), Unless the subscriber only
creates a IPv4 client. In which case they won't subscribe to
"prefix-delegated", but that signal is also not invoked for IPv4
clients.
Combine the signals in one, and pass all parameters via a new
NMDhcpClientNotfiyData payload. I feel this is nicer, to pack all
parameters together. I find this more type-aware, where we can
switch (in the callback) based on a notify-type enum, instead
of subscribing multiple signal handlers.
With l3cfg work, DHCP handling will be refactored, where this model of
having one "generic" notify signal makes more sense than here. For the
moment, it is arguably pretty much the same. Also, because NMDhcpClient
subscribes two different handlers for IPv4 and IPv6. In the future,
there will be only one notify handler, and that can cover IPv4 and IPv6
and both "state-changed" and "prefix-delegated" (and other notification
types).
Now that NetworkManager on Fedora 33 and RHEL 9 no longer writes
ifcfg-rh files by default ([1]), ifup/ifdown became less useful.
Possibly users shouldn't use it and it would be fine that new-style profiles
(keyfile) no longer work with these commands. But this is deemed as too
disruptive for users.
Note that our previous ifup/ifdown compat scripts only honored the argument
to be part of the ifcfg filename. That was not what initscripts were doing,
which called `need_config()` function that searched also the contents of
the files. With this extension, ifup/ifdown gets smarter too, to better
guess what the user might have wanted.
Extend the script by making it smarter, and to work with connection profile
names.
With this extension we further solidify ifup/ifdown as part of NetworkManager
command line API. That is problematic, because these tools pollute the
$PATH, by not having a clear NM-specific name. Also, these scripts
should only exist on Fedora/RHEL, which makes their usage non-portable
to other distros.
Also, other distros already ship different tools with name ifup/ifdown.
Extending the use of these scripts is thus undesirable, as it furthers
distro-specific commands.
Still, these arguments seem to not hold and users need to be "helped".
As Fedora users cannot be expected to unlearn "ifup" today, there is no
reason to assume they could in a few years. This likely means we will
never get rid of these scripts.
Also, if we truly would make ifup/ifdown part of NetworkManager, then a better
implementation would be that nmcli honors being called with these names.
That is not done, because nmcli's implementation currently is not as
nice to make that extension trivial (as it should be). It also would
mean to embrace ifup/ifdown officially. A shell script works well enough
as a hack.
[1] https://fedoraproject.org/wiki/Changes/NetworkManager_keyfile_instead_of_ifcfg_rhhttps://bugzilla.redhat.com/show_bug.cgi?id=1954607https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/936
As we keep track of the current name owner, use its unique name
for the D-Bus requests.
We also track when the name owner changes, so at the point when we make
the D-Bus call, the current name owner was still running. We should talk
to it directly. If at the same time, firewalld restarts, we go through
our usual tracking of the name owner and will retry -- but always
talking to the unique name.
nm-sudo and nm-dispatcher are very similar from a high level. Both are D-Bus activated
services that exit on idle and all they do, is to provide a simple D-Bus API with no
objects or properties.
Hence it's not surprising that they follow the same structure.
Rename the code to make them look more similar.
After we released the well-known name (or if we failed to ever request
it), we must exit as fast as possible, so that a new instance can
be started to serve new requests.
At that point, reject new requests because they are targeted against the
unique name, which they should not do (when talking to a D-Bus activated
service that exits on idle, it's important to talk to the well-known
name).
Also, if we receive SIGTERM, start releasing the name. We are told to
shut down, and must do so in a timely manner. Again, new requests shall
not be served by this instance.
Currently we only implmement two operations (Ping() and GetFD()). Both
complete right away. There is no need to register a pending job, if
the job does not get processed asynchronously.
In the future, we may have methods that need asynchronous processing
and where we need to register them as pending job.
If we fail to acquire the well-known name or if we already released it,
we must not accept anymore new requests.
Otherwise, requests directly targeted to the unique name will keep the
process alive, and prevent it from restarting (and serving the
well-known name). Clients really should not talk to the unique name of a
service that exits on idle. If they do, and the service is about to shut
down, then the request will be rejected. After we released the name,
there is now turning back and we should quit fast (only processing the
requests we already have).
Also, if we receive a SIGTERM, then we are requested to quit and should
do so in a timely manner. That means, we will start with releasing the
name. As the service is D-Bus activated, new requests can be served by
the next instance (or if the service is about to be disabled altogether,
they will start failing).
The "only_dirty" parameter to a remove-all() function is odd.
For one, the function is called remove-all, but depending on a parameter
it does not remove all.
Also, setting remove-all(only_dirty=TRUE) means it will remove not
everything, so passing TRUE will remove only parts. That logic seems
confusing.
Avoid that, by removing the parameter from nm_l3cfg_remove_config_all()
and add nm_l3cfg_remove_config_all_dirty().
add a NM_DISPATCHER_ACTION_DHCP_CHANGE_X() macro that can select the
right action based on a parameter.
Also rename the IPv4/IPv6 enum values, so that their naming scheme works
better with the NM_DISPATCHER_ACTION_DHCP_CHANGE_X() macro.
