C casts unconditionally force the type, and as such they don't
necessarily improve type safety, but rather overcome restrictions
from the compiler when necessary.
Casting a void pointer is unnecessary (in C), it does not make the
code more readable nor more safe. In particular for g_object_new(),
which is known to return a void pointer of the right type.
Drop such casts.
sed 's/([A-Za-z_0-9]\+ *\* *) *g_object_new/g_object_new/g' $(git grep -l g_object_new) -i
./contrib/scripts/nm-code-format-container.sh
We have:
- nm_utils_hashtable_cmp(): this does a full cmp of two hash
tables, with the intent to provide a stable sort order.
It thus takes a GCompareDataFunc() argument.
- nm_utils_hashtable_cmp_equal(): this is like nm_utils_hashtable_cmp(),
except that the caller won't get a compare value, only a boolean
value that indicates equality.
This was previously called nm_utils_hashtable_equal().
- nm_utils_hashtable_equal(): this takes a GEqualFunc function
for comparing the values for equality. It takes thus
a different kind of predicate, but otherwise is similar to
nm_utils_hashtable_cmp_equal().
This was previously called nm_utils_hash_table_equal().
Unify the naming of these functions.
Historically, keyfile read/write code was part of core, and thus
GPL-2.0+ licensed. Keyfile is the native file format for NetworkManager
connection profiles, and code to handle that should be part of libnm.
This would unlock many interesting features, like tools being able
to import/export connection profiles in the native file format.
However, libnm is LGPL-2.1+ licensed, so this is a problem.
The alternative would be to add a separate, GPL licensed library
(libnm-keyfile.so or libnm-gpl.so). However that also requires a larger
rework, because the current keyfile implementation uses internal API
from libnm-core and it would need to be reworked to only use public
API of libnm.
Relicense the code instead. According to research and "keyfile-history.sh"
script, the following individuals and companies possibly hold copyright
on the code:
<bgalvani(at)redhat.com>
<blueowl(at)centrum.cz>
<daniel(at)gnoutcheff.name>
<danw(at)redhat.com>
<dcantrell(at)redhat.com>
<dcbw(at)redhat.com>
<evan(at)ebroder.net>
<fgiudici(at)redhat.com>
<floe(at)butterbrot.org>
<j(at)bootlab.org>
<kmaraas(at)gnome.org>
<lkundrak(at)v3.sk>
<luzpaz(at)users.noreply.github.com>
<martinpitt(at)gnome.org>
<michael.i.doherty(at)intel.com>
<pavlix(at)pavlix.net>
<pmarti(at)warp.es>
<rafaelff(at)gnome.org>
<rstrode(at)redhat.com>
<tambet(at)gmail.com>
<tgraf(at)redhat.com>
<thaller(at)redhat.com>
<walters(at)verbum.org>
<yurchor(at)ukr.net>
Intel Corporation
Novell, Inc.
Red Hat, Inc.
Ximian, Inc.
Most contributors on this list agreed to relicensing according to RELICENSE.md.
The following copyright holders did not answer the request for agreeing
to relicensing:
- <j(at)bootlab.org>: no contributions were made that are related to
keyfile implementation. The script just gives a false positive.
- <pmarti(at)warp.es>: the contribution is a fix of a spelling error
(commit 6029288ffb).
- <tgraf(at)redhat.com>: the contribution to keyfile code are small
(I only identified commit 5b7503e95e).
Also, Thomas worked for Red Hat at the time.
After research, I think it's fair to conclude that everybody who holds
non-trivial copyright on the keyfile code agreed to the relicensing.
>>>
H0=a3e75f329446a93a61ca4c458a7657bd919f4fe6
commit_has_file() {
git ls-tree -r "$1" | grep -q "\\s$2"\$
}
print_commit_authors() {
git --no-pager log --full-history --follow --no-merges --pretty='format:<%ae>' $H0 -- "$1" | sort | uniq
}
print_blame_authors() {
local LAST_H
if commit_has_file $H0 "$1"; then
LAST_H=$H0
else
LAST_H="$(git log --full-history --no-merges -n1 --pretty='format:%H' $H0 -- "$1")"^1
fi
git blame --no-progress -C -C -C20 -M -M10 -e "$LAST_H" -- "$1" | sed 's/.*\(<[^>]\+@[^>]\+>\).*/\1/' | sort | uniq
}
print_grep() {
git --no-pager log -p --full-history --follow $H0 -- "$1" | grep -i '[a-z0-9]@\|author\|copyright' | sort | uniq
}
prefix() {
sed "s/^/>>>$1 /"
}
collect_all() {
for F; do
print_commit_authors "$F" | prefix 1
echo
print_blame_authors "$F" | prefix 2
echo
print_grep "$F" | prefix 3
done |
sort |
uniq |
sed 's/@/(at)/'
}
collect_all \
include/NetworkManager.h \
libnm-core/NetworkManager.h \
libnm-core/nm-dbus-interface.h \
libnm-util/NetworkManager.h \
NetworkManager.h \
;
<<<
>>>1 <alfonso.sanchez-beato(at)canonical.com>
>>>1 <bberg(at)redhat.com>
>>>1 <bgalvani(at)redhat.com>
>>>1 <caillon(at)redhat.com>
>>>1 <daniel(at)gnoutcheff.name>
>>>1 <danw(at)gnome.org>
>>>1 <dcbw(at)redhat.com>
>>>1 <dsd(at)laptop.org>
>>>1 <gcampagna(at)src.gnome.org>
>>>1 <jarteaga(at)jbeta.is>
>>>1 <jiri(at)resnulli.us>
>>>1 <jklimes(at)redhat.com>
>>>1 <jlu(at)pengutronix.de>
>>>1 <lkundrak(at)v3.sk>
>>>1 <luzpaz(at)users.noreply.github.com>
>>>1 <mvollmer(at)redhat.com>
>>>1 <pktoss(at)gmail.com>
>>>1 <radykal(at)radykal.com>
>>>1 <rml(at)novell.com>
>>>1 <rodrigo(at)gnome-db.org>
>>>1 <tambet(at)gmail.com>
>>>1 <tambet(at)ximian.com>
>>>1 <tgraf(at)redhat.com>
>>>1 <thaller(at)redhat.com>
>>>1 <thomasbechtold(at)jpberlin.de>
>>>1 <tredaelli(at)redhat.com>
>>>2 <bberg(at)redhat.com>
>>>2 <bgalvani(at)redhat.com>
>>>2 <blueowl(at)centrum.cz>
>>>2 <daniel(at)gnoutcheff.name>
>>>2 <danw(at)redhat.com>
>>>2 <dcbw(at)redhat.com>
>>>2 <jarteaga(at)jbeta.is>
>>>2 <jiri(at)resnulli.us>
>>>2 <jlu(at)pengutronix.de>
>>>2 <lkundrak(at)v3.sk>
>>>2 <luzpaz(at)users.noreply.github.com>
>>>2 <mvollmer(at)redhat.com>
>>>2 <pktoss(at)gmail.com>
>>>2 <radykal(at)radykal.com>
>>>2 <rml(at)novell.com>
>>>2 <tambet(at)gmail.com>
>>>2 <tgraf(at)redhat.com>
>>>2 <thaller(at)redhat.com>
>>>2 <thomasbechtold(at)jpberlin.de>
>>>2 <tredaelli(at)redhat.com>
>>>3 [...] Red Hat, Inc.
>>>3 Thiago Bauermann <thiago.bauermann(at)gmail.com>
>>>3 <j(at)bootlab.org>
Most contributors on this list agreed to relicensing according to RELICENSE.md.
The following copyright holders did not answer the request for agreeing to
relicensing:
- <caillon(at)redhat.com>: the only contributions are removing code.
Also, Christopher was working for Red Hat at that time.
- <j(at)bootlab.org>: the patch was trivial, but the relevant code also got
removed by commit f003ba8ef7.
- <jiri(at)resnulli.us>: a trivial contribution to the header. Also,
Jiri was working for Red Hat at that time.
- <radykal(at)radykal.com>: fix spelling error in code comment.
- <rodrigo(at)gnome-db.org>: trivial contribution of adding a #define for
NM_DBUS_PATH_CONNECTION_SETTINGS.
- <tgraf(at)redhat.com>: a trivial contribution to the header. Also,
Thomas was working fro Red Hat at that time.
- <thiago.bauermann(at)gmail.com>: add a define to the header. This
was later removed by commit 7a8f33aa3d.
While not everybody agreed to this relicensing, I think the not covered
contributions are trivial additions to our header file. Also, this file
was always part of libnm. While it had the wrong license comment, it was
never intended to be GPL licensed.
For historic reasons is NMSettingBond implemented differently from other
settings. It uses a strdict, and adds some validation on top of that.
The idea was probably to be able to treat bond options more generically.
But in practice we cannot treat them as opaque values, but need to know,
validate and understand all the options. Thus, this implementation with a
strdict is not nice.
The user can set the GObject property NM_SETTING_BOND_OPTIONS to any
strdict, and the setter performs no validation or normalization. That
is probably good, because g_object_set() cannot return an error to
signalize invalid settings. As often, we have corresponding C API like
nm_setting_bond_add_option() and nm_setting_bond_remove_option(). It
should be possible to get the same result both with the C API and with
the GObject property setting. Since there is already a way to set
certain invalid values, it does not help if the C API tries to prevent
that. That implies, that also add-option does not perform additional
validation and sets whatever the user asks.
Remove all validation from nm_setting_bond_add_option() and
nm_setting_bond_remove_option(). This validation was anyway only very
basic. It was calling nm_setting_bond_validate_option(), which can check
whether the string is (for example) and integer, but it cannot do
validation beyond one option. In most cases, the validation needs to
take into account the bond mode or other options, so validating one
option in isolation is not very useful.
Proper validation should instead be done via nm_connection_verify().
However, due to another historic oddity, that verification is very
forgiving too and doesn't reject many invalid settings when it should.
That is hard to fix, because making validation more strict can break
existing (and working) configurations. However, verify() already contains
basic validation via nm_setting_bond_validate_option(). So in the previous
behavior nm_setting_bond_add_option() would silently do nothing (only
returning %FALSE) for invalid options, while now it would add the
invalid options to the dictionary -- only to have it later fail validation
during nm_connection_verify(). That is a slight change in behavior, however it
seems preferable.
It seems preferable and acceptable because most users that call
nm_setting_bond_add_option() already understand the meaning and valid
values. Keyfile and ifcfg-rh readers are the few exceptions, which really just
parse a string dictionary, without need to understand them. But nmtui
or nmstate already know the option they want to set. They don't expect
a failure there, nor do they need the validation.
Note that this change in behavior could be dangerous for example for the
keyfile/ifcfg-rh readers, which silently ignored errors before. We
don't want them to start failing if they read invalid options from a
file, so instead let those callers explicitly pre-validate the value
and log an warning.
https://bugzilla.redhat.com/show_bug.cgi?id=1887523
The underscore somehow indicated that these would be an internal
function. Which they are in the sense that they are in "shared/nm-glib-aux/".
But they part of our internal helper functions, and in our code base
their use is no discouraged or "private.
Also, next I'll replace the function call with a macro, so, I will
have a need for the underscore name.
Rename.
Change the default DNS priority of VPNs to -50, to avoid leaking
queries out of full-tunnel VPNs.
This is a change in behavior. In particular:
- when using dns=default (i.e. no split-dns) before this patch both
VPN and the local name server were added (in this order) to
resolv.conf; the result was that depending on resolv.conf options
and resolver implementation, the name servers were tried in a
certain manner which does not prevent DNS leaks.
With this change, only the VPN name server is added to resolv.conf.
- When using a split-dns plugin (systemd-resolved or dnsmasq), before
this patch the full-tunnel VPN would get all queries except those
ending in a local domain, that would instead be directed to the
local server.
After this patch, the VPN gets all queries.
To revert to the old behavior, set the DNS priority to 50 in the
connection profile.
Previously, both nm_setting_connection_add_permission() and the GObject
property setter would merely assert that the provided values are valid
(and otherwise don't do anything). That is bad for handling errors.
For example, we use the property setter to initialize the setting from
keyfile and GVariant (D-Bus). That means, if a user provides an invalid
permissions value, we would emit a g_critical() assertion failure, but
otherwise ignore the configuration. What we instead need to do is to
accept the value, and afterwards fail verification. That way, a proper error
message can be generated.
$ mcli connection add type ethernet autoconnect no ifname bogus con-name x connection.permissions 'bogus:'
(process:429514): libnm-CRITICAL **: 12:12:00.359: permission_new: assertion 'strchr (uname, ':') == NULL' failed
(process:429514): libnm-CRITICAL **: 12:12:00.359: nm_setting_connection_add_permission: assertion 'p != NULL' failed
Connection 'x' (2802d117-f84e-44d9-925b-bfe26fd85da1) successfully added.
$ $ nmcli -f connection.permissions connection show x
connection.permissions: --
While at it, also don't track the permissions in a GSList. Tracking one
permission in a GSList requires 3 allocations (one for the user string,
one for the Permission struct, and one for the GSList struct). Instead,
use a GArray. That is still not great, because GArray cannot be embedded
inside NMSettingConnectionPrivate, so tracking one permission also
requires 3 allocations (which is really a fault of GArray). So, GArray
is not better in the common case where there is only one permissions. But even
in the worst case (only one entry), GArray is no worse than GSList.
Also change the API of nm_setting_connection_add_permission().
Previously, the function would assert that the arguments are in
a certain form (strcmp (ptype, "user") == 0), but still document
the such behaviors like regular operation ("[returns] %FALSE if @ptype
or @pitem was invalid"). Don't assert against the function arguments.
Also, if you first set the user to "fo:o", then
nm_setting_connection_add_permission() would accept it -- only at
a later phase, the property setter would assert against such values.
Also, the function would return %FALSE both if the input value was
invalid (an error) and if the value already existed. I think the
function should not treat a duplicate entry like a badly formatted
input.
Now the function does much less asserting of the arguments, but will
return %FALSE only if the values are invalid. And it will silently ignore
duplicate entries.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/636
Run:
./contrib/scripts/nm-code-format.sh -i
./contrib/scripts/nm-code-format.sh -i
Yes, it needs to run twice because the first run doesn't yet produce the
final result.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
glib-mkenums doesn't work with enums on single line
or with enums entries that span multiple lines, turn off
automatic formatting for these special cases to not break
docs generation.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
nm_utils_hwaddr_len() isn't very useful. It's documented to
only accept two possible input values (ARPHRD_ETHER and ARPHRD_INFINIBAND)
for which the respective return values are well known.
In particular, asserting that the input value is one of the two values
means it becomes harder to extend the function (and make it more useful).
Because, then the user would need to call:
#if NM_VERSION > NM_VERSION_1_XX
len = nm_utils_hwaddr_len (ARPHDR_FOO);
#else
len = 0
#endif
and then it would introduce an unnecessary run time dependency on
NM_VERSION_1_XX.
Instead, just document to return 0 if the value is not supported.
_nm_utils_hwaddr_aton() is only a wrapper around nm_utils_hexstr2bin_full().
But it abstracts the "right" parameters for what we consider a valid MAC
address and what not. As such, this function is useful.
Move it to "shared/" and replace the dupicate macro hwaddr_aton() with
it.
- only call hwaddr_aton() once per function. Also, pass it sizeof(buf)
as buffer size, it seems more correct.
- the only downside is that we now would always first parse up to 20
characters, before comparing the requested length. Previously, a
MAC address that was too long was rejected earlier (and the parsing
aborted earlier). But that is a tiny overhead, also we expect that
in common cases the MAC addresses are in fact of the right size,
then there is no difference.
nm_setting_ip_config_next_valid_dns_option() API was added in libnm 1.2, but
it was never exported in the ABI of libnm. It thus was unusable, and any user
trying to link against it would have been unable to do so.
Hide the API now entirely. It doesn't seem a very nice API. If we want to
allow the user to validate option names, we should expose such a function
to validate an option (not to fetch the next valid option from a
profile).
Fixes: 019943bb5d ('libnm-core: add dns-options property to NMSettingIPConfig')
nm_utils_hexstr2bin_full() is our general hexstr to binary parsing
method. It uses (either mandatory or optional) delimiters. Before,
if delimiters are in use, it would accept individual hexdigits.
E.g. "a:b" would be accepted as "0a:0b:.
Add an argument that prevents accepting such single digits.
Setting "active_slave" fails unless the slave is currently present and
IFF_UP. That complicates the code, because we cannot set the property
at any time, but only under the right circumstances.
But really, "active_slave" option is something for debugging. It's not
an option which should be set by NetworkManager. The right option
instead is "primary", which will tell kernel to make the slave active,
when it is ready.
Deprecate the "active_slave" option and make it an alias for "primary".
https://bugzilla.redhat.com/show_bug.cgi?id=1856640
