Reasons:
- it adds an O(1) lookup index for accessing NMIPxConfig's addresses.
Hence, operations like merge/intersect have now runtime O(n) instead
of O(n^2).
Arguably, we expect low numbers of addresses in general. For low
numbers, the O(n^2) doesn't matter and quite likely in those cases
the previous implementation was just fine -- maybe even faster.
But the simple case works fine either way. It's important to scale
well in the exceptional case.
- the tracked objects can be shared between the various NMPI4Config,
NMIP6Config instances with NMPlatform and everybody else.
- the NMPObject can be treated generically, meaning it enables code to
handle both IPv4 and IPv6, or addresses and routes. See for example
_nm_ip_config_add_obj().
- I want core to evolve to somewhere where we don't keep copies of
NMPlatformIP4Address, et al. instances. Instead they shall all be
shared. I hope this will reduce memory consumption (although tracking a
reference consumes some memory too). Also, it shortcuts nmp_object_equal()
when comparing the same object. Calling nmp_object_equal() on the
identical objects would be a common case after the hash function
pre-evaluates equality.
Eventually, every NMPlatformIP4Route, NMPlatformIP6Route,
NMPlatformIP4Address and NMPlatformIP6Address should be shared
an deduplicated via the global NMDedupMultiIndex instance.
As first proof of concept, refactor NMIP4Config to track
IPv4 routes via the shared multi_idx. There is later potential
for improvement, when we pass (deduplicated) NMPObject instances
around instead of plain NMPlatformIP4Route, which needs still
a lot of comparing and cloning.
- config->removed can be replaced by c_list_is_empty(&config->lst)
- downgrade some assertions to nm_assert(). Even without the
assert we crash a few lines later with a NULL pointer access.
That gives almost the same debuggability and discoverability
of the bug.
- use exact type signature for GAsyncReadyCallback and avoid
casting.
- when the name owner disappears, cancel all asynchronous
operations. Note how the new pacrunner instance will anyway
start without configuration, so for all intended purpose, all
pending operations are at that moment obsolete.
We must not cancel pacrunner_cancellable when the D-Bus proxy is
created. Instead, keep it around and use it later for the asynchronous
D-Bus operations.
This doesn't really matter at the moment, because the pacrunner manager
is only destroyed when NetworkManager is about to terminated. That is
the only time when we actually cancel the asynchronous request. Also,
at that time we no longer iterate the mainloop, so the pending requests
are never completed anyway.
As NMDevice now creates the NMPacrunnerManager instance
as needed, it is even more likely that the initial call
to nm_pacrunner_manager_send() will only queue (but not yet
send) the new config.
Later, when the D-Bus proxy is created, we will not get a
name-owner changed signal. We instead have to push the configuration
right away.
(cherry picked from commit 019b9fbfc0)
nm_pacrunner_manager_remove() required a "tag" argument. It was a
bug for callers trying to remove a configuration for a non-existing
tag.
That effectively means, the caller must keep track of whether a certain
"tag" is pending. The caller also must remember the tag -- a tag that he
must choose uniquely in the first place.
Turn that around and have nm_pacrunner_manager_send() return a (non
NULL) call-id. This call-id may later be used to remove the
configuration.
Apparently, previously the tracking of the "tag" was not always correct
and we hit the assertion in nm_pacrunner_manager_remove().
https://bugzilla.redhat.com/show_bug.cgi?id=1444374
(cherry picked from commit b04a9c90eb)
If a configuration does not have a path it is because we are still
sending it to pacrunner or because we failed to do so. In both cases,
we have to remove the configuration from the list.
Fixes: 3ad89223d0
(cherry picked from commit fad2cf0721)
If a VPN provides a proxy, we want to restrict the usage of that proxy
to URLs in the VPN domain. For all other connections, the proxy should
be used for all domains.
(cherry picked from commit b139552255)
Fix some issues in nm-pacrunner-manager.c:
- when adding a configuration through nm_pacrunner_manager_send(), we
kept an association between the interface name and the pacrunner
configuration object path, so that the configuration for that
interface could be removed later. Unfortunately not all
configurations have an interface associated, so we need a more
generic way to identify configurations. Introduce a new @tag
argument that serves as key to match configurations
- the interface name of the last pushed configuration was stored in
the manager private config and reused later; this could cause
issues when there are multiple outstanding D-Bus calls. The
interface is not needed anymore after the previous point.
- remove() didn't actually remove the configuration from the list
(cherry picked from commit 3ad89223d0)
Keep the include paths clean and separate. We use directories to group source
files together. That makes sense (I guess), but then we should use this
grouping also when including files. Thus require to #include files with their
path relative to "src/".
Also, we build various artifacts from the "src/" tree. Instead of having
individual CFLAGS for each artifact in Makefile.am, the CFLAGS should be
unified. Previously, the CFLAGS for each artifact differ and are inconsistent
in which paths they add to the search path. Fix the inconsistency by just
don't add the paths at all.
Pacrunner uses the interface information to implement a myIpAddress()
function which returns the first IPv4 address of the interface
associated to a proxy. That function doesn't deal with multiple
addresses per interface, and so in case of a VPN which configures
addresses on the parent interface (e.g. IPsec), we currently pass a
NULL interface. That is correct, but triggers the following assertion:
GLib-CRITICAL **: g_variant_new_variant: assertion 'value != NULL' failed
#0 g_logv () from target:/lib64/libglib-2.0.so.0
#1 g_log () from target:/lib64/libglib-2.0.so.0
#2 g_variant_new_string () from target:/lib64/libglib-2.0.so.0
#3 nm_pacrunner_manager_send (self=0xab3230, iface=iface@entry=0x0, proxy_config=proxy_config@entry=0xc83470, ip4_config=ip4_config@entry=0x7f66b4002710, ip6_config=ip6_config@entry=0x0) at nm-pacrunner-manager.c:334
#4 _set_vpn_state (self=self@entry=0xd0c120, vpn_state=vpn_state@entry=STATE_ACTIVATED, reason=reason@entry=NM_VPN_CONNECTION_STATE_REASON_NONE, quitting=quitting@entry=0) at vpn-manager/nm-vpn-connection.c:571
#5 dispatcher_pre_up_done (call_id=<optimized out>, user_data=<optimized out>) at vpn-manager/nm-vpn-connection.c:460
#6 dispatcher_done_cb (proxy=0x988870, result=<optimized out>, user_data=0xae1740) at nm-dispatcher.c:444
Ignore the interface parameter when it's NULL.
The names NMPacRunnerManager, nm_pac_runner_manager were inconsistent
with NM_PACRUNNER_MANAGER and nm-pacrunner-manager.[hc]. They should
be consistent.
It seems pacrunner project calls itself "PACrunner" or just "pacrunner",
so prefer the spelling with lower-case 'r'.
libnm-core: pac-script property in NMSettingProxy now represents the
script itself not the location. It ensures that the connection is
self contained.
nmcli: Supports loading of PAC Script via file path or written explicitly.
Unnecessary APIs have been removed from nm-setting-proxy, client like
nm-connection-editor are expected to create a PAC script snippet the load
the location of file in NM.
The API of NMProxyConfig exposes @proxies and @excludes as strv values.
There is no need to track those values internally as a GPtrArray and
then clone them in the getters (especially, since the entire NMProxyConfig
API is internal to core.
Thereby, fix a few memory leaks in add_proxy_config() and some
style fixes for { }.
As the type is called NMPacRunnerManager, the proper name for the
functions is nm_pac_runner_manager*(). Alternatively, it the type
should be NMPacrunnerManager.
nm_pacrunner_manager_send() would only fail if passed in a NULL proxy_config.
And then it would log a <info> message without details about what failed.
Just don't do that.
A new object NMPacRunnerManager has been added to manage and interact
PacRunner. It invokes both DBus methods on PacRunner DBus interface.
It stores the returned object path from CreateProxyConfiguration()
to feed as parameter to DestroyProxyCofiguration() when network goes down.
