Note how the nm_ndisc_add_*() return a boolean to indicate whether
anything changes. That is taken to decide whether to emit a changed
signal.
Previously, we would not consider all fields which are exposed
as public API.
Note that nm-ip6-config.c would care about the lifetime of NMNDiscAddress.
For that, nm_ndisc_add_address() would correctly consider a change of
the lifetime as relevant. So, this was for the most part not broken.
However, for example nm_ndisc_add_route() would ignore changes to the
gateway.
Always signal changes if anything changes at all. It's more correct
and robust.
It should never happen that we are unable to switch the namespace.
However, in case it does, we cannot just return G_SOURCE_CONTINUE,
because we will just endlessly trying to process IO without actually
reading from the socket.
This shouldn't happen, but the instance is hosed and something is
very wrong. No longer handle the socket to avoid an endless loop.
event_ready() calls ndp_callall_eventfd_handler(), which invokes
our own callback, which may invoke change notification.
At that point, it's not guaranteed that the signal handler won't
destroy the ndisc instance, which means, the "struct ndp" gets destroyed
while invoking callbacks. That's bad, because libndp is not robust
against that.
Ensure the object stays alive long enough.
Globals are bad. Don't let nmc_readline_helper() access
nm_cli.
Instead, pass nmc_config along. nmc_config albeit being
a complex struct, is much more begning:
- the configuration nmc_config is initialized early on
and afterwards immutable.
- it only contains simple fields, which affect the behavior.
- it's not a global. While passing around the complex configuration
struct, it is clear that all callpaths don't access additional
global information.
1. NetworkManager-1.14.0/shared/nm-utils/nm-shared-utils.c:1242: value_overwrite: Overwriting previous write to "ch" with value from "v".
2. NetworkManager-1.14.0/shared/nm-utils/nm-shared-utils.c:1239: assigned_value: Assigning value from "++str[0]" to "ch" here, but that stored value is overwritten before it can be used.
# 1237| if (ch >= '0' && ch <= '7') {
# 1238| v = v * 8 + (ch - '0');
# 1239|-> ch = (++str)[0];
# 1240| }
# 1241| }
Don't assign ch when it is going to be overwritten.
Even if a direct pointer comparison should be fine, use the proper
function. GVariantType documentation says:
"Two types may not be compared by value; use g_variant_type_equal()
or g_variant_type_is_subtype_of()."
This also fixes coverity warnings.
3. NetworkManager-1.14.0/libnm-core/nm-utils.c:4944: var_compare_op: Comparing "str" to null implies that "str" might be null.
4. NetworkManager-1.14.0/libnm-core/nm-utils.c:4958: var_deref_op: Dereferencing null pointer "str".
# 4956|
# 4957| /* do some very basic validation to see if this might be a JSON object. */
# 4958|-> if (str[0] == '{') {
# 4959| gsize l;
# 4960|
keyfile already supports omitting the "connection.id" and
"connection.uuid". In that case, the ID would be taken from the
keyfile's name, and the UUID was generated by md5 hashing the
full filename.
No longer do this during nm_keyfile_read(), instead let all
callers call nm_keyfile_read_ensure_*() to their liking. This is done
for two reasons:
- a minor reason is, that one day we want to expose keyfile API
as public API. That means, we also want to read keyfiles from
stdin, where there is no filename available. The implementation
which parses stdio needs to define their own way of auto-generating
ID and UUID. Note how nm_keyfile_read()'s API no longer takes a
filename as argument, which would be awkward for the stdin case.
- Currently, we only support one keyfile directory, which (configurably)
is "/etc/NetworkManager/system-connections".
In the future, we want to support multiple keyfile dirctories, like
"/var/run/NetworkManager/profiles" or "/usr/lib/NetworkManager/profiles".
Here we want that a file "foo" (which does not specify a UUID) gets the
same UUID regardless of the directory it is in. That seems better, because
then the UUID won't change as you move the file between directories.
Yes, that means, that the same UUID will be provided by multiple
files, but NetworkManager must already cope with that situation anyway.
Unfortunately, the UUID generation scheme hashes the full path. That
means, we must hash the path name of the file "foo" inside the
original "system-connections" directory.
Refactor the code so that it accounds for a difference between the
filename of the keyfile, and the profile_dir used for generating
the UUID.
Split out the functionality for auto-detecting the ID and UUID of
a connection. First of all, nm_keyfile_read() is already overcomplicated.
The next commit will require the caller to explicitly call these
functions.
In general, it's fine to pass %NULL to g_free().
However, consider:
char *
foo (void)
{
gs_free char *value = NULL;
value = g_strdup ("hi");
return g_steal_pointer (&value);
}
gs_free, gs_local_free(), and g_steal_pointer() are all inlinable.
Here the compiler can easily recognize that we always pass %NULL to
g_free(). But with the previous implementation, the compiler would
not omit the call to g_free().
Similar patterns happen all over the place:
gboolean
baz (void)
{
gs_free char *value = NULL;
if (!some_check ())
return FALSE;
value = get_value ();
if (!value)
return FALSE;
return TRUE;
}
in this example, g_free() is only required after setting @value to
non-NULL.
Note that this does increase the binary side a bit (4k for libnm, 8k
for NetworkManager, with "-O2").
