NMTST_SWAP() used memcpy() for copying the value, while NM_SWAP() uses
a temporary variable with typeof(). I think the latter is preferable.
Also, the macro is essentially doing the same thing.
It's unnecessary and makes the function unnecessarily not thread safe.
Of course, also ndp_msg_opt_route_prefix() uses static variables, so
it's still not thread safe.
Fixes: c3a4656a68 ('rdisc: libndp implementation')
We should handle kernel command line like systemd does, with its
ConditionKernelCommandLine= setting.
For example, it tokenizes words between various white space characters,
not only space. Use nm_utils_strsplit_set_full() for that.
Note that we currently don't yet have a tokenizer that supports
quotation, like systemd does. We should extend
nm_utils_strsplit_set_full() for that.
g_clear_pointer() would always cast the destroy notify function
pointer to GDestroyNotify. That means, it lost some type safety, like
GPtrArray *ptr_arr = ...
g_clear_pointer (&ptr_arr, g_array_unref);
Since glib 2.58 ([1]), g_clear_pointer() is also more type safe. But
this is not used by NetworkManager, because we don't set
GLIB_VERSION_MIN_REQUIRED to 2.58.
[1] f9a9902aac
We have nm_clear_pointer() to avoid this issue for a long time (pre
1.12.0). Possibly we should redefine in our source tree g_clear_pointer()
as nm_clear_pointer(). However, I don't like to patch glib functions
with our own variant. Arguably, we do patch g_clear_error() in
such a manner. But there the point is to make the function inlinable.
Also, nm_clear_pointer() returns a boolean that indicates whether
anything was cleared. That is sometimes useful. I think we should
just consistently use nm_clear_pointer() instead, which does always
the preferable thing.
Replace:
sed 's/\<g_clear_pointer *(\([^;]*\), *\([a-z_A-Z0-9]\+\) *)/nm_clear_pointer (\1, \2)/g' $(git grep -l g_clear_pointer) -i
I think it's preferable to use nm_clear_g_free() instead of
g_clear_pointer(, g_free). The reasons are not very strong,
but I think it is overall preferable to have a shorthand for this
frequently used functionality.
sed 's/\<g_clear_pointer *(\([^;]*\), *\(g_free\) *)/nm_clear_g_free (\1)/g' $(git grep -l g_clear_pointer) -i
Move local variables to inner scope.
Also, drop code comment that doesn't give additional information
beyond what is already plainly visible in source code.
This pull requests sets the OWE flag for an open network advertising an
OWE enabled transition BSSID. This way, hostapd will automatically
connect to the OWE secured BSSID advertised in the transition mode
information element.
Signed-off-by: David Bauer <mail@david-bauer.net>
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/442
When we receive a "InterfaceRemoved" signal, we will end up calling
set_state_down(). That emits a "state" change signal, which causes
NMDeviceWifi to unref the supplicant interface. This may already
give up the last reference, and we cleanup the supplicant state
(by again calling set_state_down()). When we return, set_state_down()
will crash because it operates on an already destroyed instance.
Avoid that by keeping a reference to the interface during set_state_down().
Fixes: b83f07916a ('supplicant: large rework of wpa_supplicant handling')
https://bugzilla.redhat.com/show_bug.cgi?id=1815058
Since ifcfg-rh doesn't write out to file the 'connection.timestamp' property
let's add it before comparing an updated connection with the plugin's reread
one otherwise the comparison operation would always fail.
The fix is not necessary for the keyfile plugin, because the reader/writer
correctly reads/writes the connection timestamp.
For many purposes, the supplicant features are not very interesting (as
they are also mostly static for a certain release/distribution). Combine
the multiple logging lines into one.
Also, sort the NMSupplCapType enum values consistently with the order
in which we log them.
Also, rename the logging output for features to match the enum name.
E.g. "FAST" instead of "EAP-FAST".
Now:
> supplicant: supported features: AP+ PMF+ FILS- P2P+ FT+ SHA384+ MESH+ FAST+ WFD+
When a device gets a prefix delegation, we call
nm_device_use_ip6_subnet() for all other devices that have IPv6
sharing enabled, which changes the current IPv6 configuration and
notifies NMPolicy. When updating the DNS configuration in NMPolicy, we
should notify all devices except the one that triggered the change.
https://bugzilla.redhat.com/show_bug.cgi?id=1488030
Configuration stages like act_stage2_config() can postpone progressing
to the next stage. Currently, when the condition that we wait for gets
satisfied, the code schedules the next stage from there.
I think that is wrong, because when we postpone from act_stage2_config(),
follow up steps of stage2 get skipped. Thus, when we are ready to progress,
the class should enter stage 2 again.
This requires that stage2 becomes reentrant and that the code reenters the
same stage.
We usually want to schedule stage2 when we just completed with the previous
stage (or, if we are currently in stage2, and want to re-enter it).
In those cases, the conditions are often right to just proceed right away.
No need to schedule the stage on an idle handler. Allow to invoke stage2
right away.
There was only API to schedule the stage on an idle handler.
Sometimes, we are just in the right situation to schedule the stage
right away. It should be possibly to avoid going through the extra hop.
For now, none of the caller makes use of this. So, there isn't any
actual change in behavior. But by adding this possibility, we may do
use in the future.
Avoid GDBusProxy, instead use GDBusConnection directly. I very much
prefer this because that way we have explicit control over what happens
on D-Bus. With GDBusProxy this is hidden under another layer of complex
code. The hardest part when using a D-Bus interface is to manage the
state via an asynchronous medium. GDBusProxy contains state about the
D-Bus interface and duplicate the state that we track. This makes it hard
to reason about things.
Rework creation of NMSupplicantInterface. Previously, a NMSupplicantInterface
had multiple initialization states. In particular, the first state would not
yet tie the interface to a certain D-Bus object path. Instead, NMSupplicantInterface
would try and retry to create the D-Bus object.
Now, NMSupplicantManager has an asynchronous method to create interface
instances. The manager only creates an interface instance after the D-Bus
path is known. That means, a NMSupplicantInterface instance is now
strongly tied to a name-owner and D-Bus path.
It follows that the state of NMSupplicantInterface can only go from STARTING,
via the supplicant states, to DOWN. Never back. That was already previously
the case that the state from DOWN was final and once the 3 initial
states were passed, the interface's state would never go back to the initial
state. Now this is more strict and more formalized. The 3 initialization states
are combined.
I think the tighter state handling simplifies users of NMSupplicantInterface.
See for example "nm-device-ethernet.c". It's still complicated, because handling
state is fundamentally difficult.
NMSupplicantManager will take care to D-Bus activate wpa_supplicant only
when necessary (poke). Previously, creating the manager instance
would always start suppliant service. Now, it's started on demand.
Add '_nm_setting_bond_get_option_or_default()' and move all the custom
policies applied by NM for bond options in there.
One such example of a custom policy is to set 'miimon' to 0 (instead of its
default value of 100) if 'arp_interval' is explicitly enabled
and 'miimon' is not.
This means removing every piece of logic from
nm_setting_bond_add_option() which used to clear out 'arp_interval' and
'arp_ip_target' if 'miimon' was set or clear out 'miimon' along with
'downdelay', 'updelay' and 'miimon' if 'arp_interval' was set.
This behaviour is a bug since the kernel allow setting any combination
of this options for bonds and NetworkManager should not limit the user
to do so.
Also use 'set_bond_attr_or_default()' instead of 'set_bond_attr()' as
the former calls '_nm_setting_bond_get_option_or_default()' to implement
the right logic to retrieve bond options according to current bond
configuration.
Don't do a linear search through all names, but use binary search.
Upside: calling nms_ifcfg_rh_utils_get_ethtool_by_name() in a loop
(once over all 60 names) is 75% faster.
Downside: when adding a new feature, we have yet another line that we
need to add. Previously, adding a new feature required adding 7 lines,
not it is 8. But we didn't add a single feature since this was added,
so that happens very seldom.
Possible downside: is this code harder to read? Now we track both how to
convert the ID to name and back. This is redundant (and thus harder to
maintain). But it's really just one extra line per feature, for which there
is a unit test. So, when adding a new NMEthtoolID it would be pretty
hard to mess this up, because of all the tests and assertions.
So, maybe it's slightly harder to read. On the other hand, it unifies
handling for ethtool and kernel names, and the code has less logic
and is more descriptive. I don't think this is actually harder to maintain
and it should be easy to see that it is correct (readability).
Otherwise, we only prune unused files when the service terminates.
Usually, NetworkManager service doesn't get restarted before shutdown
of the system (nor should it be). That means, if you create (and
destroy) a large number of software devices, the state files pile
up.
From time to time, go through the files on disk and delete those that
are no longer relevant.
In this case, "from time to time" means after we write/update state
files 100 times.
nm_manager_write_device_state() writes the device state to a file. The ifindex
is here important, because that is the identifier for the device and is also
used as file name. Return the ifindex that was used, instead of letting the
caller reimplement the knowledge which ifindex was used.
It was not very clear whether update_dns() would always set the error
output variable if (and only if) it would return false.
Rework the code to make it clearer.
Not using cleanup attribute is error prone.
In theory, a function should only return a GError if (and only if) it
signals a failure. However, for example in commit 324f67956a ('dns:
ensure to log a warning when writing /etc/resolv.conf fails') due to
a bug we was violated. In that case, it resulted in a leak.
Avoid explicit frees and use the gs_free_error cleanup attribute
instead. That would also work correctly in face of such a bug and in
general it seems preferable to explicitly assign ownership to auto
variables on the stack.
When setting "main.rc-manager=symlink" (the default) and /etc/resolv.conf
is a file, NetworkManager tries to write the file directly. When that fails,
we need to make sure to propagate the error so that we log a warning about that.
With this change:
<debug> [1583320004.3122] dns-mgr: update-dns: updating plugin systemd-resolved
<trace> [1583320004.3123] dns-sd-resolved[f9e3febb7424575d]: send-updates: start 8 requests
<trace> [1583320004.3129] dns-mgr: update-resolv-no-stub: '/var/run/NetworkManager/no-stub-resolv.conf' successfully written
<trace> [1583320004.3130] dns-mgr: update-resolv-conf: write to /etc/resolv.conf failed (rc-manager=symlink, $ERROR_REASON)
<trace> [1583320004.3132] dns-mgr: update-resolv-conf: write internal file /var/run/NetworkManager/resolv.conf succeeded
<trace> [1583320004.3133] dns-mgr: current configuration: [{ [...] }]
<warn> [1583320004.3133] dns-mgr: could not commit DNS changes: $ERROR_REASON
<info> [1583320004.3134] device (eth0): Activation: successful, device activated.
https://bugzilla.redhat.com/show_bug.cgi?id=1809181
The GSource must also be unrefed. Also, first clear the field
before invoking callbacks to the upper layers.
Fixes: 843d696e46 ('dhcp: clean source on dispatch failure')
Fix the following warning:
NetworkManager[1524461]: Source ID 3844 was not found when attempting to remove it
g_logv (log_domain=0x7f2816fa676e "GLib", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=args@entry=0x7ffe697374d0) at gmessages.c:1391
g_log (log_domain=log_domain@entry=0x7f2816fa676e "GLib", log_level=log_level@entry=G_LOG_LEVEL_CRITICAL, format=format@entry=0x7f2816fae240 "Source ID %u was not found when attempting to remove it") at gmessages.c:1432
g_source_remove (tag=519) at gmain.c:2352
nm_clear_g_source (id=<optimized out>) at ./shared/nm-glib-aux/nm-macros-internal.h:1198
dispose (object=0x55f7289b1ca0) at src/dhcp/nm-dhcp-nettools.c:1433
g_object_unref (_object=<optimized out>) at gobject.c:3303
g_object_unref (_object=0x55f7289b1ca0) at gobject.c:3232
dhcp4_cleanup (self=self@entry=0x55f728af3b20, cleanup_type=cleanup_type@entry=CLEANUP_TYPE_DECONFIGURE, release=release@entry=0) at src/devices/nm-device.c:7565
...
Fixes: 45521b1b38 ('dhcp: nettools: move to failed state if event dispatch fails')
