Most of the times we actually need a NMSecretAgentSimple typed pointer.
This way, need need to cast less.
But even if we would need to cast more, it's better to have pointers
point to the actual type, not merely to avoid shortcomings of C.
No caller cared about the NM_SECRET_AGENT_ERROR_AGENT_CANCELED reason.
In particular, because previously the requests would keep the secret-agent
instance alive, and this never happend.
Also, NM_SECRET_AGENT_ERROR_AGENT_CANCELED precicley exists for
NMSecretAgentOld:cancel_get_secrets() (as documented). During finalize
we are not cancelled -- at least not the same way as
cancel_get_secrets(). Setting NM_SECRET_AGENT_ERROR_AGENT_CANCELED
is wrong.
Anyway, we have a default error for such cases already.
The code was correct. But it's hard to follow when and whether
the child-watch-id was destroyed at the right time.
Instead, always let _auth_dialog_data_free() clear the signal handlers.
Don't let RequestData keep the parent NMSecretAgentSimple instance
alive. Previously, the code in finalize() was never actually reached.
Also, move the final callback from finalize() to dispose(). It feels
wrong to invoke callbacks from finalize().
We must actually cancel the GCancellable. Otherwise, the pending async
operations are not cancelled. _auth_dialog_write_done() doesn't care
about that, but _auth_dialog_read_done() does. It must not touch the
destroyed data, after the operation is cancelled.
Note that previously the @requests hash took the request-id as key and
the RequestData as value. Likewise, the destroy functions of the head
would destroy the key and the value.
However, RequestData also had a field "request_id". But that pointer was
not owned (nor freed) by the RequestData structure. Instead, it was
relied that the hash kept the request-id alive long enough.
That is confusing. Let RequestData own the request-id.
Also, we don't need to track a separate key. Just move the request-id
as first filed in RequestData, and use compare/hash functions that
handle that correctly (nm_pstr_*()).
Code that is internal to a source file should not have a "nm" prefix.
That is what differenciates it from declarations in header files. It
makes it clearer that these names are only defined in the current file.
Also, our implementations of virtual functions shall have the same
name as the function pointer of the VTable (or at least, it shouldn't
have a "nm" prefix).
NMPNetns instances are immutable, hence they can be easily shared
between threads. All we need, is that the stack of namespaces is
thread-local.
Also note that NMPNetns uses almost no other API, except some bits from
"shared/nm-utils/" and nm-logging. These parts are already supposed to
be thread-safe.
The only complications is that when the thread exits, we need to
destroy the NMPNetns instances. That is especially important because
they hold file descriptors. This is accomplished using pthread's
thread-specific data. An alternative would be C11 threads' tss_create(),
but not all systems that we run against support that yet. This means,
we need to link with pthreads, but we already do that anyway.
Note that glib also requires pthreads. So, we don't get an additional
dependency here.
NetworkManager is single-threaded and uses a mainloop.
However, sometimes we may need multiple threads. For example, we will
need to write sysctl values asynchronously, using the glib thread-pool.
For that to work, we also need to switch the network-namespace of the
thread-pool thread. We want to use NMPNetns for that. Hence it's better
to have NMPNetns thread-safe, instead of coming up with a duplicate
implementation. But NMPNetns may want to log, so we also need nm-logging
thread-safe.
In general, code under "shared/nm-utils" and nm-logging should be usable
from multiple threads. It's simpler to make this code thread-safe than
re-implementing it. Also, it's a bad limitation to be unable to log
from other threads. If there is an error, the best we can often do is to
log about it.
Make nm-logging thread-safe. Actually, we only need to be able to log
from multiple threads. We don't need to setup or configure logging from
multiple threads. This restriction allows us to access logging from the
main-thread without any thread-synchronization (because all changes in
the logging setup are also done from the main-thread).
So, while logging from other threads requires a mutex, logging from the
main-thread is lock-free.
Instead of having two functions nm_logging_set_syslog_identifier()
and nm_logging_set_prefix(), merge them.
They must both be called at earliest point and together. No point
in giving them the appearance that they could be called any time.
This variable has other requirements for multi-threaded access (it will
only be accessible from the main-thread). Move it to a separate global
variable to make that clearer.
The distinction between only reading static data and modifying it,
is important when making nm-logging thread-safe.
This change should make it easier to find the places where we modify
data.
@level_desc and @domain_desc are two immutable arrays with information
about logging levels and logging domains. Since they are immutable and
intialized from the start, they are pretty trival w.r.t. tread-safety and
general maintainability (code readability).
Refactor them to be separate variables.
Previously, _nm_logging_clear_platform_logging_cache was an extern variable,
and NMLinuxPlatform would set it to a function pointer at certain points.
That's unnecessary complex, also when trying to make nm-logging thread-safe,
it's just more global variables that need to be considered. Don't do it
that way, but just link in a regular function.
Since commit 9ecdba316 ('platform: create netlink messages directly
without libnl-route-3') we're unconditionally setting IFA_ADDRESS to
the peer address, even if there's no peer and it's all zeroes.
The kernel actually stopped caring somewhere around commit caeaba790
('ipv6: add support of peer address') in v3.10, but Ubuntu Touch likes
to run Android's v3.4 on some poorly supported hardware.
Fixes: 9ecdba316chttps://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/77
We already need to special handle regular settings (with secrets as
GObject properties) and VPN secrets.
Next, we will also need to special handle WireGuard peers, which can
have secrets too.
Move the code to a virtual function, so that "nm-connection.c" and
"nm-setting.c" does not have explicit per-setting knowledge.
- use cleanup attribute to free memory
- return floating reference from _nm_connection_for_each_secret().
It's more idiomatic that a function that constructs a variant and
returns it, returns a floating variant.
_nm_connection_for_each_secret() (formerly for_each_secret()) and
_nm_connection_find_secret() (formerly find_secret()) operate on a
GVariant of secrets. For that, they implement certain assumptions
of how to handle secrets. For example, it must special-case VPN settings,
because there is no generic abstraction to handle regular secret and VPN
secrets the same.
Such special casing should only be done in libnm-core, at one place.
Move the code to libnm-core as internal API.
nm_setting_get_secret_flags() looks whether we have a suitable "-flags"
data value, or whether we have a secret with that name.
In fact, we know this is a valid secret-name. Even if there are no secret-flags
and the secret (currently) does not exists. We shall not care about the
return value.
Note that nm_setting_get_secret_flags() also for non-secrets will set
the flags to "NONE", which is just what we need.
Instead of special-casing the aggregate implementation for NMSettingVpn,
delegate to a virtual function.
This will also work with other settings, that have properties/secrets
that are not GObject based properties.
While nm_setting_enumerate_values() should not be used anymore, still
extend it to make it workable also for properties that are not based on
GObject properties.
We want to emit a change notification when gendata-based settings (like
NMSettingEthtool) change. But instead of adding a separate signal, just
emit a fake "notify:name" notification.
When the parent device has a device default route (i.e. without
gateway) and we establish a VPN on top of it, 'ip route get' for the
VPN gateway returns a device route, which is the same result we get
for an unreachable VPN gateway. However it is necessary to add the
route to the gateway or otherwise it will possibly become unreachable
once the VPN gets activated.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/93
