We are going to expose some of this API in libnm.
The name "gendata" (for "generic data") is not very suited. Instead,
call the public API nm_setting_option_*(). This also brings no naming
conflict, because currently no API exists with such naming.
Rename the internal API, so that it matches the API that we are going
to expose next.
nm_g_error_matches() can be inlined and first checks whether the error
argument is not NULL. At least from the keyfile accessor functions, use
this macro, as they are called many times.
We had three callers of nm_keyfile_plugin_kf_get_integer_list(). Two
only wanted to read values in range of guint8. One, wanted to read
unsigned integers (for which nm_keyfile_plugin_kf_get_integer_list()
was not suitable).
Instead, implement a integer list reader ourself.
One change is that g_key_file_get_integer_list() would accept list elements
with a number followed by a white space and garbage ([1]). We don't do that,
so there is a change in behavior here. That seems preferable, we don't
want to accept garbage.
The error reason text from the reader now also changes, and obviously we
no longer fail for integer values larger than G_MAXINT.
[1] c9bf247eb9/glib/gkeyfile.c (L4445)
Keyfile handles GObject properties of type G_TYPE_ARRAY as a GArray
of unsigned ints. That is correct, because all our properties of this
GType happen to be of this kind.
However, then the function was using nm_keyfile_plugin_kf_set_integer_list(),
which only can handle signed integers. There was thus an assertion that all
integers were non-negative. Which, probably was also correct, because NMSettingDcb
would validate that all values of such kind are in fact positive. Anyway, that
is an unexpected limitation (if not a bug).
Fix that by handling the array as unsigned list of integers.
Also, since glib doesn't provide an API for storing lists of unsigend
integers, we have to implement our own. but that is no loss. We probably
do it better anyway.
Previously, we were preallocating a string buffer of fixed size. For guint8
we reserved 3 characters per number, which is sufficient. However, it is
not obviously sufficient. NMStrBuf would grow as needed.
Next, I will add nm_keyfile_plugin_kf_set_integer_list_uint(), where it
is more unclear how large the string can be at most. To avoid that question
from the start, it will use NMStrBuf. To keep the implementations similar,
use NMStrBuf also in this case.
Split the macros to define the setter and getter so that setters
and getters are defined by separate macros. This will be used
to define the boolean getter differently, but still using the
macro to define the setter.
Also, don't construct function names in the macro. Instead, pass
the full names as argument to the macro. This helps with the problem
where ctags/cscope is unable to locate the implementation of the
function. Since we define the function with macro, the tools still
don't recognize this as the location of the definition. But at least
when showing all occurrences of the name, it can be found.
In general, I like macros. But in this case it seems the make the code harder
to understand than it needs to be. There are repeated patterns in these declarations,
but I feel they are better recognizible by aligning the lines nicely.
Why "if (length > G_MAXUINT)"? This is never going to hit. Also,
we probably should actual missing keys handle differently from
empty lists. If @error is set, return without setting the property.
g_key_file_get_integer_list() can return %NULL without setting an error.
That is the case if the key is set to an empty value.
For X sake, this API. Read the documentation and figure out whether
the function can return %NULL without reporting an error.
Anyway, avoid the assertion failure.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/412
I think it's preferable to use nm_clear_g_free() instead of
g_clear_pointer(, g_free). The reasons are not very strong,
but I think it is overall preferable to have a shorthand for this
frequently used functionality.
sed 's/\<g_clear_pointer *(\([^;]*\), *\(g_free\) *)/nm_clear_g_free (\1)/g' $(git grep -l g_clear_pointer) -i
We should use the same "is-valid" function everywhere.
Since nm_utils_ipaddr_valid() is part of libnm, it does not qualify.
Use nm_utils_ipaddr_is_valid() instead.
Keyfile support was initially added under GPL-2.0+ license as part of
core. It was moved to "libnm-core" in commit 59eb5312a5 ('keyfile: merge
branch 'th/libnm-keyfile-bgo744699'').
"libnm-core" is statically linked with by core and "libnm". In
the former case under terms of GPL-2.0+ (good) and in the latter case
under terms of LGPL-2.1+ (bad).
In fact, to this day, "libnm" doesn't actually use the code. The linker
will probably remove all the GPL-2.0+ symbols when compiled with
gc-sections or LTO. Still, linking them together in the first place
makes "libnm" only available under GPL code (despite the code
not actually being used).
Instead, move the GPL code to a separate static library
"shared/nm-keyfile/libnm-keyfile.la" and only link it to the part
that actually uses the code (and which is GPL licensed too).
This fixes the license violation.
Eventually, it would be very useful to be able to expose keyfile
handling via "libnm". However that is not straight forward due to the
licensing conflict.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/381
