For one, these functions are not often needed. No need to define them in the
"nm-macros-internal.h" header, which is included everywhere. Move them to
"nm-shared-utils.h", which must be explicitly included.
Also, these functions are usually not called directly, but by passing their
function pointer to a sort function or similar. There is no point in having
defined in the header file.
Splitting keyfile handling in two "reader.c" and "writer.c" files
is not helpful. What is most interesting, is to see how property XYZ
is serialized to keyfile, and to verify that the parser does the
inverse. For that, it's easier if both the write_xzy() and parse_xyz()
function are beside each other, and not split accross files.
The more important reason is, that both reader and writer have their
separate handler arrays, for special handling of certain properties:
@key_parsers and @key_writers. These two should not be separate but will
be merged. Since they reference static functions, these functions must
all be in the same source file (unless, we put them into headers, which
would be unnecessary complex).
No code was changed, only moved.
The key_writers array is searched by matching the @key during
write_setting_value(). Note how write_setting_value() is called
by nm_connection_for_each_setting_value(), thus, @key is the name
of a GObject property for NMSettingIP4Config. But NMSettingIP4Config
has no property names "address-labels". Hence, this was unused
since introducing libnm-core (which never had this internal property).
With this, parsing the properties address/route (for both IPv4/IPv6)
has a runtime complexity of O(n*ln(n)).
Previously, parsing these properties was O(1), but the constant factor
was very high because for each address/route x ipv4/ipv6 combination we would
search about 2*1001 times whether there is a matching value.
Now the runtime complexity is O(n*ln(n)) for each of these 4 properties
where n is the number of entries in the keyfile.
Also note, that we only have 4 properties for which the parsing has
this complexity. Hence, parsing the entire keyfile is still O(n) + 4*O(n*ln(n))
which reduces to O(n*ln(n)). So, parsing the entire keyfile is still benign
and the logarithmic factor comes merely from sorting (which is fast).
Now, the number of supported addresses/routes is no longer limited
to 1000 (as before). Now we would accept all keys up from 0 up to
G_MAXINT32.
Like before, indexes will be automatically adjusted and gaps in the
numbering are accepted. That is convenient, if the user edits the
keyfile manually and deletes some lines. And we anyway must not change
behavior.
$ multitime -n 200 -s 0 -q ./src/settings/plugins/keyfile/tests/test-keyfile
# build with -O2 --without-more-asserts
# before:
Mean Std.Dev. Min Median Max
real 0.290+/-0.0000 0.013 0.275 0.289 0.418
user 0.284+/-0.0000 0.010 0.267 0.284 0.331
# after:
Mean Std.Dev. Min Median Max
real 0.101+/-0.0000 0.002 0.099 0.100 0.118
user 0.096+/-0.0000 0.003 0.091 0.096 0.113
sys 0.004+/-0.0000 0.002 0.001 0.004 0.009
When the D-Bus name is already taken, NM crashes in the following
way. That's because disposed object are not unexported when quitting
and so they linger in the bus-manager's list of exported objects,
causing an invalid access when a neighboring item is accessed. Instead
of just clearing the path, fully unexport the object.
The behavior of not forcefully exporting objects on quit was added in
f9ee20a7b2 ("core: explicitly unexport objects when we're done with
them"), but such behavior doesn't seem to be needed by the stated
goal.
<error> [1524062008.1886] bus-manager: fatal failure to acquire D-Bus service "org.freedesktop.NetworkManager" (3). Service already taken
<trace> [1524062008.2327] config: state: success writing state file "/var/lib/NetworkManager/NetworkManager.state"
<trace> [1524062008.2338] dns-mgr: stopping...
<info> [1524062008.2344] exiting (error)
<debug> [1524062008.2628] disposing NMManager singleton (0xce587e0)
<trace> [1524062008.2640] dns-mgr: disposing
<debug> [1524062008.2651] disposing NMDnsManager singleton (0xceb8b50)
<debug> [1524062008.2666] disposing NMFirewallManager singleton (0xceb62b0)
<debug> [1524062008.2709] disposing NMHostnameManager singleton (0xce7b370)
<trace> [1524062008.2722] dbus-object[0xce70f40]: unexport: "/org/freedesktop/NetworkManager/AgentManager"
==16381== Invalid write of size 8
==16381== at 0x42F511: c_list_unlink_stale (c-list.h:158)
==16381== by 0x42F511: c_list_unlink (c-list.h:171)
==16381== by 0x42F511: _nm_dbus_manager_obj_unexport (nm-dbus-manager.c:1135)
==16381== by 0x4C5E35: nm_dbus_object_unexport (nm-dbus-object.c:165)
==16381== by 0x5C01E9: dispose (nm-agent-manager.c:1634)
==16381== by 0x6636F37: g_object_unref (gobject.c:3303)
==16381== by 0x4BDC89: _nm_singleton_instance_destroy (nm-core-utils.c:138)
==16381== by 0x400FA85: _dl_fini (in /usr/lib64/ld-2.27.so)
==16381== by 0x7F806AB: __run_exit_handlers (in /usr/lib64/libc-2.27.so)
==16381== by 0x7F807DB: exit (in /usr/lib64/libc-2.27.so)
==16381== by 0x41DA34: main (main.c:463)
==16381== Address 0xce706a0 is 48 bytes inside a block of size 176 free'd
==16381== at 0x4C2EDAC: free (vg_replace_malloc.c:530)
==16381== by 0x6ACA3E1: g_free (gmem.c:194)
==16381== by 0x6AE2572: g_slice_free1 (gslice.c:1136)
==16381== by 0x66550AE: g_type_free_instance (gtype.c:1943)
==16381== by 0x4505F8: dispose (nm-manager.c:6867)
==16381== by 0x6636F37: g_object_unref (gobject.c:3303)
==16381== by 0x4BDC89: _nm_singleton_instance_destroy (nm-core-utils.c:138)
==16381== by 0x400FA85: _dl_fini (in /usr/lib64/ld-2.27.so)
==16381== by 0x7F806AB: __run_exit_handlers (in /usr/lib64/libc-2.27.so)
==16381== by 0x7F807DB: exit (in /usr/lib64/libc-2.27.so)
==16381== by 0x41DA34: main (main.c:463)
==16381== Block was alloc'd at
==16381== at 0x4C2DBAB: malloc (vg_replace_malloc.c:299)
==16381== by 0x6ACA2D5: g_malloc (gmem.c:99)
==16381== by 0x6AE1E36: g_slice_alloc (gslice.c:1025)
==16381== by 0x6AE247C: g_slice_alloc0 (gslice.c:1051)
==16381== by 0x6654E09: g_type_create_instance (gtype.c:1848)
==16381== by 0x66376C7: g_object_new_internal (gobject.c:1799)
==16381== by 0x6638E14: g_object_new_with_properties (gobject.c:1967)
==16381== by 0x66399D0: g_object_new (gobject.c:1639)
==16381== by 0x5D6F18: nm_settings_new (nm-settings.c:1897)
==16381== by 0x4514B4: constructed (nm-manager.c:6489)
==16381== by 0x66378FA: g_object_new_internal (gobject.c:1839)
==16381== by 0x6638E14: g_object_new_with_properties (gobject.c:1967)
https://github.com/NetworkManager/NetworkManager/pull/96
Introduce a new ifcfg-rh variable ACD_TIMEOUT that stores the exact
value of ipv4.dad-timeout without rounding. We still write the
initscripts-compatible ARPING_WAIT variable, and read it when
ACD_TIMEOUT is missing.
Print a warning whenever we find a IP conflict on the network. In the
future we may export a flag on the device or send a signal so that
clients can notify the user of the conflict.
Don't return an error from nm_arping_manager_start_probe() since it is
currently useless and the arping-manager already prints the failure
reason. Also, drop a log print from add_address().
NMArpingManager previously spawned an arping process for each
probed/announced address and watched it. This has the disadvantage of
being inefficient and also that for small timeouts we can't be sure
that arping actually started the probe.
Switch to an implementation that doesn't need to spawn external
processes, by using the n-acd code [1] currently imported in our
source tree. The long term plan is that n-acd will become a shared
library we can link against.
The file is still called nm-arping-manager for lazyness, even if a
better name would be nm-acd-manager.
[1] https://github.com/nettools/n-acd/https://bugzilla.redhat.com/show_bug.cgi?id=1507864
If the main loop is quit before the timeout expires, we leave the
timeout source running on the main loop context. Since we usually
create the main loop using the default context, the source will fire
on the next main loop we create during the test.
Therefore, destroy the timeout source if it is still active.
Fixes: 766f31507b
The README states that a kernel >= 3.0 is enough, however
CLOCK_BOOTTIME is only available since kernel 3.15.
Fall back to CLOCK_MONOTONIC when CLOCK_BOOTTIME is not available.
See: https://github.com/nettools/n-acd/pull/3
Signed-off-by: Beniamino Galvani <bgalvani@redhat.com>
During _new_active_connection() we just create the NMActiveConnection
instance to proceed with authorization. The caller might not even
authorize, so we must not touch the device yet.
Do that only later.
Often, functions perform a series of steps, and when they fail,
they bail out. It's simpler if the code is structured that way,
so you can read it from top to bottom and whenever something is
wrong, either return directly (or goto a cleanup label at the
bottom).
From the D-Bus layer, no specific-object is represented by "/". We
should early on normalize such values to NULL, and not expect or
handle them later (like during _new_active_connection()).
Merge _new_vpn_active_connection() into _new_active_connection(). It was the
only caller, and it is simpler to have all the code visible at one place.
That also shows, that the device argument is ignored and not handled.
Ensure that no device is specified for VPN type activations.
Also, in _add_and_activate_auth_done(), always steal the connection
from active's user-data. Otherwise, the lifetime of the connection
is extended until active gets destroyed. For example, if we would leak
active, we would also leak connection that way.
- pass is-vpn to _new_active_connection(). It is confusing that _new_active_connection()
would re-determine whether a connection is a VPN type, although that was already
established previously. The confusing part is: will they come to the
same result? Why? What if not?
Instead pass it as argument and assert that we got it right.
- the check for existing connections should look whether there is an existing
active connection of type NMVpnConnection. Instead, what matters is,
- do we have a connection of type VPN (otherwise, don't even bother
to search for existing-ac)
- is the connection already active?
Checking whether the connection is already active, and ask backwards
whether it's of type NMVpnConnection is odd, maybe even wrong in
some cases.
- there are only two callers of validate_activation_request(). One of them,
might already lookup the device before calling the validate function.
Safe to looking up again. But this is not only an optimization, more importantly,
it feels odd to first lookup a device, and then later look it up again. Are
we guaranteed to use the same path? Why? Just avoid that question.
- re-order some error checking for missing device, so that it is clearer.
- use cleanup attribute to handle return value and drop the "goto error".
NMDBusObject already gets this right, by calling nm_dbus_utils_get_property(),
which calls g_dbus_gvalue_to_gvariant(), which correctly converts NULL
object paths to "/".
We already rely on that elsewhere. No need for this workaround.
