From 900aa016b1351a9b002178aedf5834d876ff5101 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 1 Jun 2015 16:34:16 +0200 Subject: [PATCH 1/5] ifcfg-rh: log warning when loading of connection fails connection_from_file() used to log a warning about failure, but only when an @error argument was given. update_connection() didn't ensure that in several cases, so we would not log any failure reason when an ifcfg file failed to read. This behavior of controlling logging by passing @error (or not) is unexpected. Instead, refactor the code so that the caller can do appropriate logging. Another reason for this refactoring is that PARSE_WARNING() does not mention the file for which the failure is and uses some extra indention that looks wrong. IOW, connection_from_file() doesn't have the context to give the logging line a proper formatting. --- src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c | 11 ++++++++--- src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.h | 3 ++- src/settings/plugins/ifcfg-rh/plugin.c | 6 +++++- src/settings/plugins/ifcfg-rh/reader.c | 13 ++++--------- src/settings/plugins/ifcfg-rh/reader.h | 3 ++- 5 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c b/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c index 06a7f21039..a6324469cc 100644 --- a/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c +++ b/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.c @@ -201,7 +201,8 @@ files_changed_cb (NMInotifyHelper *ih, NMIfcfgConnection * nm_ifcfg_connection_new (NMConnection *source, const char *full_path, - GError **error) + GError **error, + gboolean *out_ignore_error) { GObject *object; NMConnection *tmp; @@ -211,13 +212,17 @@ nm_ifcfg_connection_new (NMConnection *source, g_assert (source || full_path); + if (out_ignore_error) + *out_ignore_error = FALSE; + /* If we're given a connection already, prefer that instead of re-reading */ if (source) tmp = g_object_ref (source); else { tmp = connection_from_file (full_path, &unhandled_spec, - error); + error, + out_ignore_error); if (!tmp) return NULL; @@ -376,7 +381,7 @@ commit_changes (NMSettingsConnection *connection, */ filename = nm_settings_connection_get_filename (connection); if (filename) { - reread = connection_from_file (filename, NULL, NULL); + reread = connection_from_file (filename, NULL, NULL, NULL); if (reread) { same = nm_connection_compare (NM_CONNECTION (connection), reread, diff --git a/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.h b/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.h index 328e58f5f2..44e0298772 100644 --- a/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.h +++ b/src/settings/plugins/ifcfg-rh/nm-ifcfg-connection.h @@ -48,7 +48,8 @@ GType nm_ifcfg_connection_get_type (void); NMIfcfgConnection *nm_ifcfg_connection_new (NMConnection *source, const char *full_path, - GError **error); + GError **error, + gboolean *out_ignore_error); const char *nm_ifcfg_connection_get_unmanaged_spec (NMIfcfgConnection *self); const char *nm_ifcfg_connection_get_unrecognized_spec (NMIfcfgConnection *self); diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c index 15e702ef8b..0d4ad1be35 100644 --- a/src/settings/plugins/ifcfg-rh/plugin.c +++ b/src/settings/plugins/ifcfg-rh/plugin.c @@ -213,6 +213,7 @@ update_connection (SCPluginIfcfg *self, const char *new_unrecognized = NULL, *old_unrecognized = NULL; gboolean unmanaged_changed = FALSE, unrecognized_changed = FALSE; const char *uuid; + gboolean ignore_error = FALSE; g_return_val_if_fail (!source || NM_IS_CONNECTION (source), NULL); g_return_val_if_fail (full_path || source, NULL); @@ -222,13 +223,16 @@ update_connection (SCPluginIfcfg *self, /* Create a NMIfcfgConnection instance, either by reading from @full_path or * based on @source. */ - connection_new = nm_ifcfg_connection_new (source, full_path, error); + connection_new = nm_ifcfg_connection_new (source, full_path, &local, &ignore_error); if (!connection_new) { /* Unexpected failure. Probably the file is invalid? */ if ( connection && !protect_existing_connection && (!protected_connections || !g_hash_table_contains (protected_connections, connection))) remove_connection (self, connection); + if (!source && !ignore_error) + _LOGW ("loading \"%s\" fails: %s", full_path, local ? local->message : "(unknown reason)"); + g_propagate_error (error, local); return NULL; } diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c index ed49f40e66..6dcb1a3641 100644 --- a/src/settings/plugins/ifcfg-rh/reader.c +++ b/src/settings/plugins/ifcfg-rh/reader.c @@ -4889,18 +4889,13 @@ done: NMConnection * connection_from_file (const char *filename, char **out_unhandled, - GError **error) + GError **error, + gboolean *out_ignore_error) { - gboolean ignore_error = FALSE; - NMConnection *conn; - - conn = connection_from_file_full (filename, NULL, NULL, + return connection_from_file_full (filename, NULL, NULL, out_unhandled, error, - &ignore_error); - if (error && *error && !ignore_error) - PARSE_WARNING ("%s", (*error)->message); - return conn; + out_ignore_error); } NMConnection * diff --git a/src/settings/plugins/ifcfg-rh/reader.h b/src/settings/plugins/ifcfg-rh/reader.h index 70e9ce4e24..2096ffc40e 100644 --- a/src/settings/plugins/ifcfg-rh/reader.h +++ b/src/settings/plugins/ifcfg-rh/reader.h @@ -28,7 +28,8 @@ NMConnection *connection_from_file (const char *filename, char **out_unhandled, - GError **error); + GError **error, + gboolean *out_ignore_error); char *uuid_from_file (const char *filename); From 4ef8c0c90ce213303505e2eab3db4f8741ad6d94 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 1 Jun 2015 15:28:43 +0200 Subject: [PATCH 2/5] ifcfg-rh: also read alias file for dhcp connections Previously, if the main ifcfg file doesn't define any static ip addresses, any alias files would be ignored. We should also allow alias files with (pure) 'dhcp' connections, just like initscripts do. Reported-by: Marek Hulan --- src/settings/plugins/ifcfg-rh/reader.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c index 6dcb1a3641..ccbcdb07ab 100644 --- a/src/settings/plugins/ifcfg-rh/reader.c +++ b/src/settings/plugins/ifcfg-rh/reader.c @@ -1180,16 +1180,14 @@ read_aliases (NMSettingIPConfig *s_ip4, const char *filename, const char *networ GDir *dir; char *dirname, *base; shvarFile *parsed; - NMIPAddress *base_addr; + NMIPAddress *base_addr = NULL; GError *err = NULL; g_return_if_fail (s_ip4 != NULL); g_return_if_fail (filename != NULL); - if (nm_setting_ip_config_get_num_addresses (s_ip4) == 0) - return; - - base_addr = nm_setting_ip_config_get_address (s_ip4, 0); + if (nm_setting_ip_config_get_num_addresses (s_ip4) > 0) + base_addr = nm_setting_ip_config_get_address (s_ip4, 0); dirname = g_path_get_dirname (filename); g_return_if_fail (dirname != NULL); From 8be9e832b59d5fb4cece9b76d7e6bdf43d1f5445 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 1 Jun 2015 14:35:59 +0200 Subject: [PATCH 3/5] ifcfg-rh: refactor utils_should_ignore_file() to return early --- src/settings/plugins/ifcfg-rh/utils.c | 44 ++++++++++++--------------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/utils.c b/src/settings/plugins/ifcfg-rh/utils.c index 87f4aba3e5..5519aac716 100644 --- a/src/settings/plugins/ifcfg-rh/utils.c +++ b/src/settings/plugins/ifcfg-rh/utils.c @@ -27,6 +27,7 @@ #include "nm-core-internal.h" #include "nm-macros-internal.h" #include "NetworkManagerUtils.h" +#include "gsystem-local-alloc.h" #include "utils.h" #include "shvar.h" @@ -151,41 +152,34 @@ check_suffix (const char *base, const char *tag) gboolean utils_should_ignore_file (const char *filename, gboolean only_ifcfg) { - char *base; - gboolean ignore = TRUE; - gboolean is_ifcfg = FALSE; - gboolean is_other = FALSE; + gs_free char *base = NULL; g_return_val_if_fail (filename != NULL, TRUE); base = g_path_get_basename (filename); - g_return_val_if_fail (base != NULL, TRUE); /* Only handle ifcfg, keys, and routes files */ - if (!strncmp (base, IFCFG_TAG, strlen (IFCFG_TAG))) - is_ifcfg = TRUE; - - if (only_ifcfg == FALSE) { - if ( !strncmp (base, KEYS_TAG, strlen (KEYS_TAG)) - || !strncmp (base, ROUTE_TAG, strlen (ROUTE_TAG)) - || !strncmp (base, ROUTE6_TAG, strlen (ROUTE6_TAG))) - is_other = TRUE; + if (strncmp (base, IFCFG_TAG, strlen (IFCFG_TAG)) != 0) { + if (only_ifcfg) + return TRUE; + else if ( strncmp (base, KEYS_TAG, strlen (KEYS_TAG)) != 0 + && strncmp (base, ROUTE_TAG, strlen (ROUTE_TAG)) != 0 + && strncmp (base, ROUTE6_TAG, strlen (ROUTE6_TAG)) != 0) + return TRUE; } /* But not those that have certain suffixes */ - if ( (is_ifcfg || is_other) - && !check_suffix (base, BAK_TAG) - && !check_suffix (base, TILDE_TAG) - && !check_suffix (base, ORIG_TAG) - && !check_suffix (base, REJ_TAG) - && !check_suffix (base, RPMNEW_TAG) - && !check_suffix (base, AUGNEW_TAG) - && !check_suffix (base, AUGTMP_TAG) - && !check_rpm_temp_suffix (base)) - ignore = FALSE; + if ( check_suffix (base, BAK_TAG) + || check_suffix (base, TILDE_TAG) + || check_suffix (base, ORIG_TAG) + || check_suffix (base, REJ_TAG) + || check_suffix (base, RPMNEW_TAG) + || check_suffix (base, AUGNEW_TAG) + || check_suffix (base, AUGTMP_TAG) + || check_rpm_temp_suffix (base)) + return TRUE; - g_free (base); - return ignore; + return FALSE; } char * From 2e87df84088295be52f68c074cb8799f1d17da44 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 1 Jun 2015 17:09:25 +0200 Subject: [PATCH 4/5] ifcfg-rh: escape colon in generated filename A colon indicates an alias file. It should be escaped. --- src/settings/plugins/ifcfg-rh/writer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/settings/plugins/ifcfg-rh/writer.c b/src/settings/plugins/ifcfg-rh/writer.c index d5f26a14b0..27f1c70599 100644 --- a/src/settings/plugins/ifcfg-rh/writer.c +++ b/src/settings/plugins/ifcfg-rh/writer.c @@ -2530,7 +2530,7 @@ escape_id (const char *id) while (*p) { if (*p == ' ') *p = '_'; - else if (strchr ("\\][|/=()!", *p)) + else if (strchr ("\\][|/=()!:", *p)) *p = '-'; p++; } From 0aed4e2388a4c8ce0f82d87a24ed5e66b76fa4f7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 1 Jun 2015 18:03:31 +0200 Subject: [PATCH 5/5] ifcfg-rh: better detect alias files Alias files have a ':' to separate the base name from their alias. But we didn't always ensure not to write-out files without colon, and also initscripts doesn't have that restriction. We should detect alias files and handle them properly (e.g. by reloading the base file). This fixes an error that a `nmcli con load` would have tried to load the alias file. Also extend load_connection() to support passing filenames other then the base file. We only have to handle this in plugin.c. Inside reader.c we always have the normalized base filename. Or detection of alias files only looks whether the filename has a ':' and whether a corresponding base file exists. --- src/settings/plugins/ifcfg-rh/plugin.c | 48 +++++++++--------------- src/settings/plugins/ifcfg-rh/utils.c | 51 +++++++++++++++++++------- src/settings/plugins/ifcfg-rh/utils.h | 3 +- 3 files changed, 57 insertions(+), 45 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c index 0d4ad1be35..5dd32af83a 100644 --- a/src/settings/plugins/ifcfg-rh/plugin.c +++ b/src/settings/plugins/ifcfg-rh/plugin.c @@ -425,25 +425,13 @@ ifcfg_dir_changed (GFileMonitor *monitor, gpointer user_data) { SCPluginIfcfg *plugin = SC_PLUGIN_IFCFG (user_data); - char *path, *base, *ifcfg_path; + char *path, *ifcfg_path; NMIfcfgConnection *connection; path = g_file_get_path (file); - if (utils_should_ignore_file (path, FALSE)) { - g_free (path); - return; - } - _LOGD ("ifcfg_dir_changed(%s) = %d", path, event_type); - - base = g_file_get_basename (file); - if (utils_is_ifcfg_alias_file (base, NULL)) { - /* Alias file changed. Get the base ifcfg file from it */ - ifcfg_path = utils_get_ifcfg_from_alias (path); - } else { - /* Given any ifcfg, keys, or routes file, get the ifcfg file path */ - ifcfg_path = utils_get_ifcfg_path (path); - } + ifcfg_path = utils_detect_ifcfg_path (path, FALSE); + _LOGD ("ifcfg_dir_changed(%s) = %d // %s", path, event_type, ifcfg_path ? ifcfg_path : "(none)"); if (ifcfg_path) { connection = find_by_path (plugin, ifcfg_path); switch (event_type) { @@ -462,7 +450,6 @@ ifcfg_dir_changed (GFileMonitor *monitor, g_free (ifcfg_path); } g_free (path); - g_free (base); } static void @@ -546,18 +533,14 @@ read_connections (SCPluginIfcfg *plugin) filenames = g_ptr_array_new_with_free_func (g_free); while ((item = g_dir_read_name (dir))) { - char *full_path; - - if (utils_should_ignore_file (item, TRUE)) - continue; - if (utils_is_ifcfg_alias_file (item, NULL)) - continue; + char *full_path, *real_path; full_path = g_build_filename (IFCFG_DIR, item, NULL); - if (!utils_get_ifcfg_name (full_path, TRUE)) - g_free (full_path); - else - g_ptr_array_add (filenames, full_path); + real_path = utils_detect_ifcfg_path (full_path, TRUE); + + if (real_path) + g_ptr_array_add (filenames, real_path); + g_free (full_path); } g_dir_close (dir); @@ -629,20 +612,25 @@ load_connection (NMSystemConfigInterface *config, SCPluginIfcfg *plugin = SC_PLUGIN_IFCFG (config); NMIfcfgConnection *connection; int dir_len = strlen (IFCFG_DIR); + char *ifcfg_path; if ( strncmp (filename, IFCFG_DIR, dir_len) != 0 || filename[dir_len] != '/' || strchr (filename + dir_len + 1, '/') != NULL) return FALSE; - if (utils_should_ignore_file (filename + dir_len + 1, TRUE)) + /* get the real ifcfg-path. This allows us to properly + * handle load command using a route-* file etc. */ + ifcfg_path = utils_detect_ifcfg_path (filename, FALSE); + if (!ifcfg_path) return FALSE; - connection = find_by_path (plugin, filename); - update_connection (plugin, NULL, filename, connection, TRUE, NULL, NULL); + connection = find_by_path (plugin, ifcfg_path); + update_connection (plugin, NULL, ifcfg_path, connection, TRUE, NULL, NULL); if (!connection) - connection = find_by_path (plugin, filename); + connection = find_by_path (plugin, ifcfg_path); + g_free (ifcfg_path); return (connection != NULL); } diff --git a/src/settings/plugins/ifcfg-rh/utils.c b/src/settings/plugins/ifcfg-rh/utils.c index 5519aac716..a793288d91 100644 --- a/src/settings/plugins/ifcfg-rh/utils.c +++ b/src/settings/plugins/ifcfg-rh/utils.c @@ -224,6 +224,12 @@ utils_get_ifcfg_name (const char *file, gboolean only_ifcfg) } \ } G_STMT_END + /* Do not detect alias files and return 'eth0:0' instead of 'eth0'. + * Unfortunately, we cannot be sure that our files don't contain colons, + * so we cannot reject files with colons. + * + * Instead, you must not call utils_get_ifcfg_name() with an alias file + * or files that are ignored. */ MATCH_TAG_AND_RETURN (name, IFCFG_TAG); if (!only_ifcfg) { MATCH_TAG_AND_RETURN (name, KEYS_TAG); @@ -419,26 +425,43 @@ utils_is_ifcfg_alias_file (const char *alias, const char *ifcfg) } char * -utils_get_ifcfg_from_alias (const char *alias) +utils_detect_ifcfg_path (const char *path, gboolean only_ifcfg) { - char *base, *ptr, *ifcfg = NULL; + gs_free char *base = NULL; + char *ptr, *ifcfg = NULL; - g_return_val_if_fail (alias != NULL, NULL); + g_return_val_if_fail (path != NULL, NULL); - base = g_path_get_basename (alias); - g_return_val_if_fail (base != NULL, NULL); + if (utils_should_ignore_file (path, only_ifcfg)) + return NULL; - if (utils_is_ifcfg_alias_file (base, NULL)) { - ifcfg = g_strdup (alias); - ptr = strrchr (ifcfg, ':'); - if (ptr) - *ptr = '\0'; - else { + base = g_path_get_basename (path); + + if (strncmp (base, IFCFG_TAG, STRLEN (IFCFG_TAG)) == 0) { + if (base[STRLEN (IFCFG_TAG)] == '\0') + return NULL; + if (utils_is_ifcfg_alias_file (base, NULL)) { + ifcfg = g_strdup (path); + ptr = strrchr (ifcfg, ':'); + if (ptr && ptr > ifcfg) { + *ptr = '\0'; + if (g_file_test (ifcfg, G_FILE_TEST_EXISTS)) { + /* the file has a colon, so it is probably an alias. + * To be ~more~ certain that this is an alias file, + * check whether a corresponding base file exists. */ + if (only_ifcfg) { + g_free (ifcfg); + return NULL; + } + return ifcfg; + } + } g_free (ifcfg); - ifcfg = NULL; } + return g_strdup (path); } - g_free (base); - return ifcfg; + if (only_ifcfg) + return NULL; + return utils_get_ifcfg_path (path); } diff --git a/src/settings/plugins/ifcfg-rh/utils.h b/src/settings/plugins/ifcfg-rh/utils.h index 445437c48b..547bfcb2d7 100644 --- a/src/settings/plugins/ifcfg-rh/utils.h +++ b/src/settings/plugins/ifcfg-rh/utils.h @@ -59,7 +59,8 @@ gboolean utils_has_complex_routes (const char *filename); gboolean utils_ignore_ip_config (NMConnection *connection); gboolean utils_is_ifcfg_alias_file (const char *alias, const char *ifcfg); -char *utils_get_ifcfg_from_alias (const char *alias); + +char *utils_detect_ifcfg_path (const char *path, gboolean only_ifcfg); #endif /* _UTILS_H_ */