GNOME Bugzilla – Bug 750558
[review] add write support to NMConfig [th/nm-config-intern-bgo750558]
Last modified: 2015-07-02 14:12:56 UTC
Add an internal NetworkManager API to write global configuration values by extending NMConfig. This solution is intended for storing individual settings in a keyfile of NetworkManager. It is not efficient, so only use it for a reasonable low number of parameters (and reasonable size of values). Branch contains other NMConfig patches as well.
> libnm/utils: add _nm_utils_strv_find_first() function + * Returns: index of first occurance or -1 if @needle is not found in @list. s/occurrance/occurrence/ +gssize +_nm_utils_strv_find_first (char **list, gssize len, const char *needle) +{ + gssize i; - return valid_strings[i] != NULL; + g_return_val_if_fail (needle, -1); needle == NULL is supported by the function when len > 0, maybe move this check inside "else if (len < 0)" ? > config: read configuration directory "/usr/lib/NetworkManager/conf.d" +# /usr/lib/NetworkManager/conf.d/, the latter file is shaddowed and thus ignored. shadowed + can be prevented by adding a file <literal>/ect/NetworkManager/conf.d/<replaceable>name</replaceable>.conf</literal>. + In this case, the file from the etc configuration shaddows the file from the s/ect/etc/ shadows
(In reply to Beniamino Galvani from comment #1) thanks, fixed and repushed.
> config: read configuration directory "/usr/lib/NetworkManager/conf.d" > contrib/rpm: update spec file to install configuration snippets in "/usr/lib/NetworkManager/conf.d" Please drop the %config tags from the files in libdir. Those files are not configuration from user's perspective and I presume rpmlint would complain too. Also, shouldn't the load order be: /usr/lib/NetworkManager/NetworkManager.conf /etc/NetworkManager/NetworkManager.conf /usr/lib/NetworkManager/conf.d/* /etc/NetworkManager/conf.d/* In that case we could move the conf.d/10-ibft-plugin.conf file. > config: add write support for NMConfig ... > The changes are persistet into a file under persisted ... > When storing the internal configuration to disc, we also remember disk Also, pushed two fixups.
(In reply to Lubomir Rintel from comment #3) > > config: read configuration directory "/usr/lib/NetworkManager/conf.d" > > contrib/rpm: update spec file to install configuration snippets in "/usr/lib/NetworkManager/conf.d" > > Please drop the %config tags from the files in libdir. Those files are not > configuration from user's perspective and I presume rpmlint would complain > too. fixed. > Also, shouldn't the load order be: > > /usr/lib/NetworkManager/NetworkManager.conf > /etc/NetworkManager/NetworkManager.conf > /usr/lib/NetworkManager/conf.d/* > /etc/NetworkManager/conf.d/* > > In that case we could move the conf.d/10-ibft-plugin.conf file. How about a different idea: we could add a pseudo plugin "no-ibft". If you configure plugin=ifcfg-rh, it actually means "plugin=ifcfg-rh,ibft". "no-ibft" would would allow users to explicitly disable it. Then there is no need for "10-ibft-plugin.conf".
(In reply to Thomas Haller from comment #4) > (In reply to Lubomir Rintel from comment #3) > How about a different idea: we could add a pseudo plugin "no-ibft". If you > configure plugin=ifcfg-rh, it actually means "plugin=ifcfg-rh,ibft". > "no-ibft" would would allow users to explicitly disable it. > Then there is no need for "10-ibft-plugin.conf". new commit: >> settings: enable "ibft" plugin by default together with "ifcfg-rh" How is that?
Repushed. Reworked > settings: enable "ibft" plugin by default together with "ifcfg-rh" and there is a new commit: > config: add write support to atomic-sections
> config: add write support for NMConfig _merge_keyfiles() doesn't check for a NULL @keyfile_intern (which can happen when the internal conf file doesn't contain any .intern section and thus intern_config_read() returns NULL). Maybe intern_config_read() should always return a non-NULL keyfile. > config: add write support to atomic-sections + g_object_class_install_property + (object_class, PROP_ATOMIC_SECTION_PREFIXES, + g_param_spec_boxed (NM_CONFIG_ATOMIC_SECTION_PREFIXES, "", "", + G_TYPE_STRV, + G_PARAM_READWRITE | + G_PARAM_CONSTRUCT_ONLY | + G_PARAM_STATIC_STRINGS)); There isn't any get_property method and so the property should be G_PARAM_WRITABLE only. > libnm: consider ordering for _nm_keyfile_equals() + for (i =0; groups[i] && groups_b[i] && !strcmp (groups[i], groups_b[i]); i++) missing space ^ > config: log configuration at startup and on reload + _LOG ("config-data[%p]: %lu groups", self, (long unsigned) groups_len); (unsigned long) ?
(In reply to Beniamino Galvani from comment #7) > > config: add write support for NMConfig > > _merge_keyfiles() doesn't check for a NULL @keyfile_intern (which can > happen when the internal conf file doesn't contain any .intern section > and thus intern_config_read() returns NULL). Maybe > intern_config_read() should always return a non-NULL keyfile. Oh right... actually, there are all kind of places where NULL is accepted as keyfile_intern. I'd rather only fix _merge_keyfiles() to accept NULL, where this was forgotten. Ok? fixed the rest, and repushed.
> test: add nmtst_assert_success() util + if (!success || error) + g_error ("(%s:%i) FAILUE success=%s, error=%s", success, error && error->message ? error->message : "(no error)"); The 'file' and 'line' arguments are missing; s/FAILUE/FAILURE/ > libnm: add keyfile utility functions +void +_nm_keyfile_copy (GKeyFile *dst, GKeyFile *src) +{ + gs_strfreev char **groups = NULL; + guint g, k; + + groups = g_key_file_get_groups (src, NULL); + for (g = 0; groups[g]; g++) { Since we don't check for src != NULL, g_key_file_get_groups() could return NULL after a failed assertion. +gboolean +_nm_keyfile_a_contains_all_in_b (GKeyFile *kf_a, GKeyFile *kf_b) +{ + [...] + groups = g_key_file_get_groups (kf_a, NULL); + for (i = 0; groups && groups[i]; i++) { + gs_strfreev char **keys = NULL; + + keys = g_key_file_get_keys (kf_a, groups[i], NULL, NULL); + if (keys) { + for (j = 0; keys[j]; j++) { Maybe save a indentation level by checking for keys != NULL directly in the for loop condition (as for groups)? > libnm: consider ordering for _nm_keyfile_equals() - _nm_keyfile_equals (c0_k1, c0_k1_c2_k3); + _nm_keyfile_equals (c0_k1, c0_k1_c2_k3, TRUE); - _nm_keyfile_equals (c0_k1, k0_c1_k2); + _nm_keyfile_equals (c0_k1, k0_c1_k2, TRUE); g_assert (_nm_keyfile_equals (...)) ? > config: add macros NM_CONFIG_GET_DATA and NM_CONFIG_GET_DATA_ORIG This can be replaced as well: ./src/nm-manager.c:2461: value = nm_config_data_get_connection_default (nm_config_get_data (nm_config_get ()), > config: use nm_config_data_get_value_boolean() - value = nm_config_data_get_value (NM_CONFIG_GET_DATA_ORIG, - IFUPDOWN_KEY_FILE_GROUP, IFUPDOWN_KEY_FILE_KEY_MANAGED); - if (value) { - gboolean manage_well_known; - - manage_well_known = !strcmp (value, "true") || !strcmp (value, "1"); - priv->unmanage_well_known = !manage_well_known; - g_free (value); - } + if (nm_config_data_get_value_boolean (NM_CONFIG_GET_DATA_ORIG, + IFUPDOWN_KEY_FILE_GROUP, IFUPDOWN_KEY_FILE_KEY_MANAGED, + FALSE)) + priv->unmanage_well_known = TRUE; Shouldn't this be: priv->unmanage_well_known = FALSE ? > config: add write support for NMConfig + /* signal the absense the value. That means, we must propagate the s/absense/absence/
> config: add write support for NMConfig +static GKeyFile * +intern_config_read (const char *filename, + GKeyFile *keyfile_conf, + gboolean *out_needs_rewrite) + [...] + if (!*filename) { + if (out_needs_rewrite) + *out_needs_rewrite = FALSE; + return NULL; + } + [...] +out: + g_key_file_unref (keyfile); + + *out_needs_rewrite = needs_rewrite; Is out_needs_rewrite (allow-none)? If not, the first check is not needed. + if (g_key_file_has_key (keyfile_intern, group, key_base, NULL)) { + /* There is also have a matching key_base entry. Skip processing s/have// > config: don't log warning in nm_config_keyfile_get_boolean() Maybe move this just after the commit it fixes (config: add nm_config_parse_boolean() function). > config: fix usage of g_key_file_get_value() vs. g_key_file_get_string() + * the raw value (g_key_file_get_value()), and do the paring ourselves. */ parsing > config/test: add test for set_values() + if (g_strcmp0 (value, expected_value)) { + g_error ("(%s:%d) invalid value in config-data %s.%s = %s%s%s (instead of %s%s%s)", + file, line, group, key, + value ? "\"" : "", + value ? value : "(null)", + value ? "\"" : "", + expected_value ? "\"" : "", + expected_value ? expected_value : "(null)", + expected_value ? "\"" : ""); NM_PRINT_FMT_QUOTED() ?
Thanks. Fixed all comments and repushed.
I reordered the commits, so that adding write-support comes last.
> config: fix order of processing [connection] sections in NMConfig +# you can overwrite individual settings in a file loaded +# previously. But note that this does not bump the priority +# of the section, i.e. [connection.ord.0.1] still has a pretty +# low priority and is shaddowed by [connection.ord.2.1]. shadowed > config/test: add test for set_values() #include <unistd.h> #include <glib.h> +#include <stdio.h> If this is really required, move it near #include <unistd.h> > test: add nmtst_assert_success() util Pushed a trivial compile fixup. > config: fix setting default configuration for 'main.plugins' + /* create a default configuration file. */ + keyfile = nm_config_create_keyfile (); + + plugins_default = g_strsplit (CONFIG_PLUGINS_DEFAULT, ",", -1); + if (plugins_default && plugins_default[0]) + nm_config_keyfile_set_string_list (keyfile, "main", "plugins", (const char *const*) plugins_default, -1); + g_strfreev (plugins_default); Does this mean that if someone has a "plugins+=" option in a snippet but doesn't have "plugins=" defined in NM.conf, the option in the snippet will be merged with the default compile-time values? I think that this is a change in behavior.
(In reply to Beniamino Galvani from comment #13) > Does this mean that if someone has a "plugins+=" option in a snippet > but doesn't have "plugins=" defined in NM.conf, the option in the > snippet will be merged with the default compile-time values? I think > that this is a change in behavior. indeed it changes behavior in that case. But I think it is more correct and it will likely not affect many users (because up to now we always installed a NetworkManager.conf that explicitly sets plugins=. So yes, but I would still do it. Sounds ok? also, added 4 new commits: 4c33bb6 config: only handle 'option+' and 'option-' keys for known settings ab980bb libnm: add @deep_copy argument to _nm_utils_strv_to_slist() and f9b3d8b libnm: expose strv utils function in internal header nm-core-internal.h dc4aecc fixup! config: refactor processing of 'option+' and 'option-' config
(In reply to Thomas Haller from comment #14) > indeed it changes behavior in that case. > > But I think it is more correct and it will likely not affect many users > (because up to now we always installed a NetworkManager.conf that explicitly > sets plugins=. > > So yes, but I would still do it. Sounds ok? I was one of the affected users :) But anyway, it's not a common situation, so I'm fine with it. > config: only handle 'option+' and 'option-' keys for known settings Maybe I'm missing it, but is there any test to check that option+ and option- are ignored for unknown settings? If not, it would be useful. > config: only handle 'option+' and 'option-' keys for known settings + for (iter_val = old_val; iter_val && *iter_val; iter_val++) { + if ( last_char != '-' + || _nm_utils_strv_find_first (new_val, -1, *iter_val) < 0) + g_ptr_array_add (new, g_strdup (*iter_val)); + } + for (iter_val = new_val; iter_val && *iter_val; iter_val++) { + /* don't add duplicates. That means an "option=a,b"; "option+=a,c" results in "option=a,b,c" */ + if ( last_char == '+' + && _nm_utils_strv_find_first (old_val, -1, *iter_val) < 0) indentation of && and || conditions The rest looks good.
merged to master. http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=65753dbc133f77ccd5531288aab9a9e80da3b786 http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=05db3ee08a30edc503e2b9033993ee3d2cbf019f