Most of these functions did not ever return failure. The functions
were assertin that the input was valid (and then returned a special
value). But they did not fail under regular conditions.
Fix the gtk-doc for some of these to not claim to be able to fail.
For some (like nm_setting_vlan_add_priority_str() and
nm_setting_vlan_get_priority()), actually let them fail for valid
input (instead of asserting).
(cherry picked from commit b5793b74ca)
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
Merge the function pointer get_func() into to_dbus_fcn().
Previously, get_func() as handled separately from to_dbus_fnc()
(formerly synth_func()). The notion was that synth-func would syntetize
properties that are D-Bus only. But that distinction does not seem
very helpful to me.
Instaed, we want to convert a property to D-Bus. Period. The
implementation should be handled uniformly. Hence, now that is
all done by property_to_dbus().
Note that property_to_dbus() is also called as default implementation
for compare-property. At least, for properties that are backed by a
GObject property.
"libnm-core" implements common functionality for "NetworkManager" and
"libnm".
Note that clients like "nmcli" cannot access the internal API provided
by "libnm-core". So, if nmcli wants to do something that is also done by
"libnm-core", , "libnm", or "NetworkManager", the code would have to be
duplicated.
Instead, such code can be in "libnm-libnm-core-{intern|aux}.la".
Note that:
0) "libnm-libnm-core-intern.la" is used by libnm-core itsself.
On the other hand, "libnm-libnm-core-aux.la" is not used by
libnm-core, but provides utilities on top of it.
1) they both extend "libnm-core" with utlities that are not public
API of libnm itself. Maybe part of the code should one day become
public API of libnm. On the other hand, this is code for which
we may not want to commit to a stable interface or which we
don't want to provide as part of the API.
2) "libnm-libnm-core-intern.la" is statically linked by "libnm-core"
and thus directly available to "libnm" and "NetworkManager".
On the other hand, "libnm-libnm-core-aux.la" may be used by "libnm"
and "NetworkManager".
Both libraries may be statically linked by libnm clients (like
nmcli).
3) it must only use glib, libnm-glib-aux.la, and the public API
of libnm-core.
This is important: it must not use "libnm-core/nm-core-internal.h"
nor "libnm-core/nm-utils-private.h" so the static library is usable
by nmcli which couldn't access these.
Note that "shared/nm-meta-setting.c" is an entirely different case,
because it behaves differently depending on whether linking against
"libnm-core" or the client programs. As such, this file must be compiled
twice.
(cherry picked from commit af07ed01c0)
The "from" part is like a key for the egress/ingress priority map.
Extend nm_setting_vlan_remove_priority_str_by_value() to accept only the
"from" part when finding and deleting value. This allows for:
$ nmcli connection modify "$PROFILE" -vlan.ingress-priority-map '4:'
$ nmcli connection modify "$PROFILE" -vlan.ingress-priority-map '4:*'
to fuzzy match the value to remove.
The same code is used by nmcli. Obviously, clients also need to
parse string representations.
That begs the question whether this should be public API of libnm.
Maybe, but don't decide that now, just reuse the code internally via
"shared/nm-libnm-core-utils.h".
- avoid the memory allocations by not using g_strsplit().
- add a helper function priority_map_parse_str(). This will
be used later, to avoid allocating a NMVlanQosMapping
result, when we don't need it on the heap.
- for the priority mappings, the "from" part is the key and must
be unique. As such, it would make sense to say
$ nmcli connection modify "$PROFILE" -vlan.ingress-priority-map '1:*'
or
$ nmcli connection modify "$PROFILE" -vlan.ingress-priority-map '1:'
to delete any mapping for that priority, regardless of the "to" part.
Add an option to leave the "to" part unspecified. This will be used
later.
Order the code in our common way. No other changes.
- ensure to include the main header first (directly after
"nm-default.h").
- reorder function definitions: get_property(), set_property(),
*_init(), *_new(), finalize(), *_class_init().
NMSetting internally already tracked a list of all proper GObject properties
and D-Bus-only properties.
Rework the tracking of the list, so that:
- instead of attaching the data to the GType of the setting via
g_type_set_qdata(), it is tracked in a static array indexed by
NMMetaSettingType. This allows to find the setting-data by simple
pointer arithmetic, instead of taking a look and iterating (like
g_type_set_qdata() does).
Note, that this is still thread safe, because the static table entry is
initialized in the class-init function with _nm_setting_class_commit().
And it only accessed by following a NMSettingClass instance, thus
the class constructor already ran (maybe not for all setting classes,
but for the particular one that we look up).
I think this makes initialization of the metadata simpler to
understand.
Previously, in a first phase each class would attach the metadata
to the GType as setting_property_overrides_quark(). Then during
nm_setting_class_ensure_properties() it would merge them and
set as setting_properties_quark(). Now, during the first phase,
we only incrementally build a properties_override GArray, which
we finally hand over during nm_setting_class_commit().
- sort the property infos by name and do binary search.
Also expose this meta data types as internal API in nm-setting-private.h.
While not accessed yet, it can prove beneficial, to have direct (internal)
access to these structures.
Also, rename NMSettingProperty to NMSettInfoProperty to use a distinct
naming scheme. We already have 40+ subclasses of NMSetting that are called
NMSetting*. Likewise, NMMetaSetting* is heavily used already. So, choose a
new, distinct name.
Previously, each (non abstract) NMSetting class had to register
its name and priority via _nm_register_setting().
Note, that libnm-core.la already links against "nm-meta-setting.c",
which also redundantly keeps track of the settings name and gtype
as well.
Re-use NMMetaSettingInfo also in libnm-core.la, to track this meta
data.
The goal is to get rid of private data structures that track
meta data about NMSetting classes. In this case, "registered_settings"
hash. Instead, we should have one place where all this meta data
is tracked. This was, it is also accessible as internal API,
which can be useful (for keyfile).
Note that NMSettingClass has some overlap with NMMetaSettingInfo.
One difference is, that NMMetaSettingInfo is const, while NMSettingClass
is only initialized during the class_init() method. Appart from that,
it's mostly a matter of taste, whether we attach meta data to
NMSettingClass, to NMMetaSettingInfo, or to a static-array indexed
by NMMetaSettingType.
Note, that previously, _nm_register_setting() was private API. That
means, no user could subclass a functioning NMSetting instance. The same
is still true: NMMetaSettingInfo is internal API and users cannot access
it to create their own NMSetting subclasses. But that is almost desired.
libnm is not designed, to be extensible via subclassing, nor is it
clear why that would be a useful thing to do. One day, we should remove
the NMSetting and NMSettingClass definitions from public headers. Their
only use is subclassing the types, which however does not work.
While libnm-core was linking already against nm-meta-setting.c,
nm_meta_setting_infos was unreferenced. So, this change increases
the binary size of libnm and NetworkManager (1032 bytes). Note however
that roughly the same information was previously allocated at runtime.
- Don't use @parent_class name. This local variable (and @object_class) is
the class instance up-cast to the pointer types of the parents. The point
here is not that it is the direct parent. The point is, that it's the
NMSettingClass type.
Also, it can only be used inconsistently, in face of NMSettingIP4Config,
who's parent type is NMSettingIPConfig. Clearly, inside
nm-setting-ip4-config.c we wouldn't want to use the "parent_class"
name. Consistently rename @parent_class to @setting_class.
- Also rename the pointer to the own class to @klass. "setting_class" is also the
wrong name for that, because the right name would be something like
"setting_6lowpan_class".
However, "klass" is preferred over the latter, because we commonly create new
GObject implementations by copying an existing one. Generic names like "klass"
and "self" inside a type implementation make that simpler.
- drop useless comments like
/* virtual functions */
/* Properties */
It's better to logically and visually structure the code, and avoid trival
remarks about that. They only end up being used inconsistently. If you
even need a stronger visual separator, then an 80 char /****/ line
should be preferred.
We commonly don't use the glib typedefs for char/short/int/long,
but their C types directly.
$ git grep '\<g\(char\|short\|int\|long\|float\|double\)\>' | wc -l
587
$ git grep '\<\(char\|short\|int\|long\|float\|double\)\>' | wc -l
21114
One could argue that using the glib typedefs is preferable in
public API (of our glib based libnm library) or where it clearly
is related to glib, like during
g_object_set (obj, PROPERTY, (gint) value, NULL);
However, that argument does not seem strong, because in practice we don't
follow that argument today, and seldomly use the glib typedefs.
Also, the style guide for this would be hard to formalize, because
"using them where clearly related to a glib" is a very loose suggestion.
Also note that glib typedefs will always just be typedefs of the
underlying C types. There is no danger of glib changing the meaning
of these typedefs (because that would be a major API break of glib).
A simple style guide is instead: don't use these typedefs.
No manual actions, I only ran the bash script:
FILES=($(git ls-files '*.[hc]'))
sed -i \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>\( [^ ]\)/\1\2/g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\> /\1 /g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>/\1/g' \
"${FILES[@]}"
constructor functions are ugly, because code is running before
main() starts. Instead, as the registration code for NMSetting types
is insid the GType constructor, we just need to ensure at the
right place, that the GType was created.
The right place here is _register_settings_ensure_inited(), because
that is called before we need the registration information.
A "vlan" setting can optionally have an ethernet setting.
However, ifcfg-rh reader always adds such a setting, because
well... Anyway, the result is that writing a VLAN setting
without ethernet section will yield a different result
on re-read.
Let's have normalization add the ethernet setting, so that
two we consistently have it present.
In some situations, we want strict checking of errors, for example when
NetworkManager receives a new connection from a client, the connection
must make sense as a whole (and since NetworkManager service is backward
compatible to the clients and not the other way around, there is no
excuse for sending invalid data to the server).
In other situations, we want a best-effort behavior. Like when
NetworkManager sends a connection to its clients, those clients
want to extract as many properties as they understand, but in order
to be forward compatible against newer server versions, invalid
or unknown properties must be accepted.
Previously, a mixture of both was done. Some issues caused a failure
to create a new NMSetting, other invalid parts were just silently
ignored or triggered a g_warning() in glib.
Now allow for both. When doing strict-validation, be more strict and
reject all unknown properties and catch when the user sets an invalid
argument. On the other hand, allow for a best-effort mode that
effectively cannot fail and will return a new NMSetting instance.
For now, add NMSettingParseFlags so that the caller can choose the
old behavior, strict parsing, or best effort.
This patch doesn't have any externally visible change except that
no more g_warnings will be emitted.
On NM 1.0 connections were created by default without the REORDER_HDR
flag, but then due to a bug in platform code (fixed in [1]), the
kernel interface always had the flag set.
Now that the setting is honored, users upgrading to the new version of
NM will see a change from the previous behavior, since interfaces will
not have REORDER_HDR and this will certainly break functionality.
The only solution here seems to be to ignore the REORDER_HDR variable
in ifcfg files (since it never had any effect) and introduce a new
NO_REORDER_HDR option for the VLAN_FLAGS variable which allows to turn
the flag off. The consequence is that the flag will be set for all old
connections.
This change introduces an incompatibility with initscripts, however is
necessary to avoid breaking user functionality upon upgrade.
Connections created through NetworkManager will still be parsed
correctly by initscripts (since we always write the REORDER_HDR
variable).
[1] db62fc9d72 ("platform: fix adding VLAN flags")
https://bugzilla.gnome.org/show_bug.cgi?id=762626
- All internal source files (except "examples", which are not internal)
should include "config.h" first. As also all internal source
files should include "nm-default.h", let "config.h" be included
by "nm-default.h" and include "nm-default.h" as first in every
source file.
We already wanted to include "nm-default.h" before other headers
because it might contains some fixes (like "nm-glib.h" compatibility)
that is required first.
- After including "nm-default.h", we optinally allow for including the
corresponding header file for the source file at hand. The idea
is to ensure that each header file is self contained.
- Don't include "config.h" or "nm-default.h" in any header file
(except "nm-sd-adapt.h"). Public headers anyway must not include
these headers, and internal headers are never included after
"nm-default.h", as of the first previous point.
- Include all internal headers with quotes instead of angle brackets.
In practice it doesn't matter, because in our public headers we must
include other headers with angle brackets. As we use our public
headers also to compile our interal source files, effectively the
result must be the same. Still do it for consistency.
- Except for <config.h> itself. Include it with angle brackets as suggested by
https://www.gnu.org/software/autoconf/manual/autoconf.html#Configuration-Headers
- "gsystem-local-alloc.h" and <gio/gio.h> are already included via
"nm-default.h". No need to include them separately.
- include "nm-macros-internal.h" via "nm-default.h" and drop all
explict includes.
- in the modified files, ensure that we always include "config.h"
and "nm-default.h" first. As second, include the header file
for the current source file (if applicable). Then follow external
includes and finally internal nm includes.
- include nm headers inside source code files with quotes
- internal header files don't need to include default headers.
They can savely assume that "nm-default.h" is already included
and with it glib, nm-glib.h, nm-macros-internal.h, etc.
Point in case:
# ip link add link dummy0 name dummy0.vlan type vlan id 4095
RTNETLINK answers: Numerical result out of range
This potentially causes existing (invalid) connections to disappear
as they now fail verification.
Instead of adjusting the range of the GObject property
NM_SETTING_VLAN_ID, reject it during vlan. This is a bit more
forgiving to an older client that isn't aware of this new restriction,
so he can first set the value without raising a critical warning.
In update_connection(), pickup the configuration of
the vlan interface from platform and create the proper
NMSettingVlan setting.
And during stage1, configure the flags of the device.
Also, change all the ingress/egress mappings at once
instead of having a netlink request for each mapping.
Also, ensure we *clear* all other mappings so that
only those are set, that were configured (done by
the *gress_reset_all argument).
We changed the default value of MSettingVlan:flags from 0 to
1 (NM_VLAN_FLAG_REORDER_HEADERS). That means, that old libnm
clients will not serialize 0 (their default).
This change broke the D-Bus API. The D-Bus API allows to omit a value
when meaning the default value. That means, we cannot change the
default value (in the D-Bus API!) without breaking previous assumptions.
A newer libnm version should treat a missing flags argument as the
old default value and thus preserve the original default value (in the
D-Bus API).
This has the downside that for the future we will continue to treat a missing
value as the old default value (0), and in order to get the new default
value (1), the client must explicitly set the flags.
We also must restore the original default value in libnm-glib.
libnm-glib does not support _nm_setting_class_override_property()
and thus it must keep thinking that the default value for the GObject
property continues to be 0. Otherwise, it would not serialize a 1, which
a new libnm would now interpret as 0.
https://bugzilla.redhat.com/show_bug.cgi?id=1250225
Fixes: 687b651598
We changed the default value for the NMSettingVlan:flags from 0 to
1 (NM_VLAN_FLAG_REORDER_HEADERS). That means, we will no longer serialize
a value of 1 over D-Bus.
This breaks older libnm clients, which treat a missing flags property as
the old default (0).
-- old clients here means: clients that still use an older version of libnm
or clients that don't use libnm, but depend on the previous default value
in the D-Bus API.
Enforce to always serialize the flags properties. This workaround has almost
no downsides except that for new clients we serialize more then absolutely
necessary.
But it ensures that old clients still receive the proper value.
https://bugzilla.redhat.com/show_bug.cgi?id=1250225
Fixes: 687b651598
The kernel defaults REORDER_HDR to 1 when creating a new VLAN, but
NetworkManager's VLAN flags property defaulted to 0. Thus REORDER_HDR was not
set for NM-created VLANs with default values.
We want to match the kernel default, so we change the default value for the
vlan.flags property. However, we do not want to change the flags for existing
connections if the property is missing in connection files. Thus we have to
update plugins for that. We also make sure that vlan.flags is always written
by 'keyfile' when the value is default. That way new connections have flags
property explicitly written and it will be loaded as expected.
https://bugzilla.redhat.com/show_bug.cgi?id=1250225
The localization headers are now included via "nm-default.h".
Also fixes several places, where we wrongly included <glib/gi18n-lib.h>
instead of <glib/gi18n.h>. For example under "clients/" directory.
Error: RESOURCE_LEAK (CWE-772): [#def10]
NetworkManager-0.9.11.0/libnm-core/nm-setting-vlan.c:225: alloc_fn: Storage is returned from allocation function "priority_map_new_from_str".
NetworkManager-0.9.11.0/libnm-core/nm-setting-vlan.c:154:4: alloc_fn: Storage is returned from allocation function "g_malloc0".
NetworkManager-0.9.11.0/libnm-core/nm-setting-vlan.c:154:4: var_assign: Assigning: "p" = "g_malloc0(8UL)".
NetworkManager-0.9.11.0/libnm-core/nm-setting-vlan.c:164:2: return_alloc: Returning allocated memory "p".
NetworkManager-0.9.11.0/libnm-core/nm-setting-vlan.c:225: var_assign: Assigning: "item" = storage returned from "priority_map_new_from_str(map, str)".
NetworkManager-0.9.11.0/libnm-core/nm-setting-vlan.c:226: leaked_storage: Variable "item" going out of scope leaks the storage it points to.
Error: RESOURCE_LEAK (CWE-772): [#def11]
NetworkManager-0.9.11.0/libnm-core/nm-utils.c:2056: alloc_fn: Storage is returned from allocation function "crypto_make_des_aes_key".
NetworkManager-0.9.11.0/libnm-core/crypto.c:405:2: alloc_fn: Storage is returned from allocation function "g_malloc0".
NetworkManager-0.9.11.0/libnm-core/crypto.c:405:2: var_assign: Assigning: "key" = "g_malloc0(digest_len + 1U)".
NetworkManager-0.9.11.0/libnm-core/crypto.c:407:2: noescape: Resource "key" is not freed or pointed-to in function "crypto_md5_hash".
NetworkManager-0.9.11.0/libnm-core/crypto.c:769:24: noescape: "crypto_md5_hash(char const *, gssize, char const *, gssize, char *, gsize)" does not free or save its pointer parameter "buffer".
NetworkManager-0.9.11.0/libnm-core/crypto.c:415:2: return_alloc: Returning allocated memory "key".
NetworkManager-0.9.11.0/libnm-core/nm-utils.c:2056: var_assign: Assigning: "key" = storage returned from "crypto_make_des_aes_key("DES-EDE3-CBC", &salt[0], salt_len, in_password, &key_len, NULL)".
NetworkManager-0.9.11.0/libnm-core/nm-utils.c:2057: leaked_storage: Variable "key" going out of scope leaks the storage it points to.
Add "---dbus---" sections to the NMSetting property docs, in the same
style as the plugin docs, parse them out into a file
"nm-setting-docs-overrides.xml", and use them to override the GObject
property docs in nm-setting-docs.xml.
This lets us put more D-Bus-specific information in the setting docs,
without cluttering up the property docs, and it also lets us document
dbus-only properties.
Move the settings/plugins doc generation from libnm-util to
libnm-core, since libnm-util isn't being updated for all new
properties.
With this commit, the keyfile and ifcfg-rh documentation is basically
unchanged, except that deprecated properties are now gone, and new
properties have been added, and the sections are in a different order.
(generate-plugin-docs.pl just outputs the settings in Makefile order,
and they were unsorted in libnm-util, but are sorted in libnm-core).
The settings documentation used for nm-settings.5, the D-Bus API docs,
and the nmcli help is changed a bit more at this point, and mostly for
the worse, since the libnm-core setting properties don't match up with
the D-Bus API as well as the libnm-util ones do. To be fixed...
(I also removed the "plugins docs" line in each plugin docs comment
block while moving them, since those blocks will be used for more than
just plugins soon, and it's sort of obvious anyway.)
Libraries need to include <gi18n-lib.h>, not <gi18n.h>, so that _()
will get defined to "dgettext (GETTEXT_DOMAIN, string)" rather than
"gettext (string)" (which will use the program's default domain, which
works fine for programs in the NetworkManager tree, but not for
external users). Likewise, we need to call bindtextdomain() so that
gettext can find the translations if the library is installed in a
different prefix from the program using it (and
bind_textdomain_codeset(), so it will know the translations are in
UTF-8 even if the locale isn't).
(The fact that no one noticed this was broken before is because the
libraries didn't really start returning useful translated strings much
until 0.9.10, and none of the out-of-tree clients have been updated to
actually show those strings to users yet.)
config.h should be included from every .c file, and it should be
included before any other include. Fix that.
(As a side effect of how I did this, this also changes us to
consistently use "config.h" rather than <config.h>. To the extent that
it matters [which is not much], quotes are more correct anyway, since
we're talking about a file in our own build tree, not a system
include.)
