From 269c15e8733a97776c764564c1f1607da5cc3332 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jan 2019 09:09:37 +0100 Subject: [PATCH] keyfile: various refactoring and restructure nm_keyfile_read() - in nm_keyfile_read(), unify _read_setting() and _read_setting_vpn_secret() in they way they are called (that is, they no longer return any value and don't accept any arguments aside @info). - use cleanup attributes - use nm_streq() instead of strcmp(). - wrap lines that have multiple statements or conditions. --- libnm-core/nm-keyfile-utils.c | 4 +- libnm-core/nm-keyfile-utils.h | 1 - libnm-core/nm-keyfile.c | 263 ++++++++++++++++-------------- shared/nm-utils/nm-shared-utils.c | 2 +- 4 files changed, 145 insertions(+), 125 deletions(-) diff --git a/libnm-core/nm-keyfile-utils.c b/libnm-core/nm-keyfile-utils.c index bc9da61485..ff12fa1063 100644 --- a/libnm-core/nm-keyfile-utils.c +++ b/libnm-core/nm-keyfile-utils.c @@ -48,7 +48,7 @@ nm_keyfile_plugin_get_alias_for_setting_name (const char *setting_name) g_return_val_if_fail (setting_name != NULL, NULL); for (i = 0; i < G_N_ELEMENTS (alias_list); i++) { - if (strcmp (setting_name, alias_list[i].setting) == 0) + if (nm_streq (setting_name, alias_list[i].setting)) return alias_list[i].alias; } return NULL; @@ -62,7 +62,7 @@ nm_keyfile_plugin_get_setting_name_for_alias (const char *alias) g_return_val_if_fail (alias != NULL, NULL); for (i = 0; i < G_N_ELEMENTS (alias_list); i++) { - if (strcmp (alias, alias_list[i].alias) == 0) + if (nm_streq (alias, alias_list[i].alias)) return alias_list[i].setting; } return NULL; diff --git a/libnm-core/nm-keyfile-utils.h b/libnm-core/nm-keyfile-utils.h index 46a6d56432..18d5dce831 100644 --- a/libnm-core/nm-keyfile-utils.h +++ b/libnm-core/nm-keyfile-utils.h @@ -90,4 +90,3 @@ const char *nm_keyfile_key_decode (const char *key, char **out_to_free); #endif /* __NM_KEYFILE_UTILS_H__ */ - diff --git a/libnm-core/nm-keyfile.c b/libnm-core/nm-keyfile.c index b3c4cdd53b..cec09c9f38 100644 --- a/libnm-core/nm-keyfile.c +++ b/libnm-core/nm-keyfile.c @@ -106,17 +106,18 @@ static void setting_alias_parser (KeyfileReaderInfo *info, NMSetting *setting, const char *key) { const char *setting_name = nm_setting_get_name (setting); - char *s; const char *key_setting_name; + gs_free char *s = NULL; s = nm_keyfile_plugin_kf_get_string (info->keyfile, setting_name, key, NULL); - if (s) { - key_setting_name = nm_keyfile_plugin_get_setting_name_for_alias (s); - g_object_set (G_OBJECT (setting), - key, key_setting_name ?: s, - NULL); - g_free (s); - } + if (!s) + return; + + key_setting_name = nm_keyfile_plugin_get_setting_name_for_alias (s); + g_object_set (G_OBJECT (setting), + key, + key_setting_name ?: s, + NULL); } static void @@ -129,7 +130,7 @@ sriov_vfs_parser (KeyfileReaderInfo *info, NMSetting *setting, const char *key) int i; keys = nm_keyfile_plugin_kf_get_keys (info->keyfile, setting_name, &n_keys, NULL); - if (!keys || n_keys == 0) + if (n_keys == 0) return; vfs = g_ptr_array_new_with_free_func ((GDestroyNotify) nm_sriov_vf_unref); @@ -622,17 +623,16 @@ ip_address_or_route_parser (KeyfileReaderInfo *info, NMSetting *setting, const c gs_free char *gateway = NULL; gs_unref_ptrarray GPtrArray *list = NULL; gs_strfreev char **keys = NULL; - gsize i_keys, keys_len; + gsize i_keys, n_keys; gs_free IPAddrRouteBuildListData *build_list = NULL; gsize i_build_list, build_list_len = 0; - keys = nm_keyfile_plugin_kf_get_keys (info->keyfile, setting_name, &keys_len, NULL); - - if (keys_len == 0) + keys = nm_keyfile_plugin_kf_get_keys (info->keyfile, setting_name, &n_keys, NULL); + if (n_keys == 0) return; /* first create a list of all relevant keys, and sort them. */ - for (i_keys = 0; i_keys < keys_len; i_keys++) { + for (i_keys = 0; i_keys < n_keys; i_keys++) { const char *s_key = keys[i_keys]; gint32 key_idx; gint8 key_type; @@ -641,7 +641,7 @@ ip_address_or_route_parser (KeyfileReaderInfo *info, NMSetting *setting, const c continue; if (G_UNLIKELY (!build_list)) - build_list = g_new (IPAddrRouteBuildListData, keys_len - i_keys); + build_list = g_new (IPAddrRouteBuildListData, n_keys - i_keys); build_list[build_list_len].s_key = s_key; build_list[build_list_len].key_idx = key_idx; @@ -786,7 +786,6 @@ mac_address_parser (KeyfileReaderInfo *info, NMSetting *setting, const char *key const char *p, *mac_str; gs_free guint8 *buf_arr = NULL; guint buf_len = 0; - gsize length; tmp_string = nm_keyfile_plugin_kf_get_string (info->keyfile, setting_name, key, NULL); @@ -819,6 +818,7 @@ mac_address_parser (KeyfileReaderInfo *info, NMSetting *setting, const char *key if (!buf_arr) { gs_free int *tmp_list = NULL; + gsize length; /* Old format; list of ints */ tmp_list = nm_keyfile_plugin_kf_get_integer_list (info->keyfile, setting_name, key, &length, NULL); @@ -879,9 +879,10 @@ read_hash_of_string (GKeyFile *file, NMSetting *setting, const char *key) const char *const*iter; const char *setting_name = nm_setting_get_name (setting); gboolean is_vpn; + gsize n_keys; - keys = nm_keyfile_plugin_kf_get_keys (file, setting_name, NULL, NULL); - if (!keys || !*keys) + keys = nm_keyfile_plugin_kf_get_keys (file, setting_name, &n_keys, NULL); + if (n_keys == 0) return; if ( (is_vpn = NM_IS_SETTING_VPN (setting)) @@ -902,7 +903,7 @@ read_hash_of_string (GKeyFile *file, NMSetting *setting, const char *key) if (!g_object_class_find_property (G_OBJECT_GET_CLASS (setting), name)) nm_setting_vpn_add_data_item (NM_SETTING_VPN (setting), name, value); } else { - if (strcmp (name, "interface-name")) + if (!nm_streq (name, "interface-name")) nm_setting_bond_add_option (NM_SETTING_BOND (setting), name, value); } } @@ -1433,7 +1434,9 @@ team_config_parser (KeyfileReaderInfo *info, NMSetting *setting, const char *key gs_free_error GError *error = NULL; conf = nm_keyfile_plugin_kf_get_string (info->keyfile, setting_name, key, NULL); - if (conf && conf[0] && !nm_utils_is_json_object (conf, &error)) { + if ( conf + && conf[0] + && !nm_utils_is_json_object (conf, &error)) { handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, _("ignoring invalid team configuration: %s"), error->message); @@ -1455,7 +1458,7 @@ qdisc_parser (KeyfileReaderInfo *info, NMSetting *setting, const char *key) qdiscs = g_ptr_array_new_with_free_func ((GDestroyNotify) nm_tc_qdisc_unref); keys = nm_keyfile_plugin_kf_get_keys (info->keyfile, setting_name, &n_keys, NULL); - if (!keys || n_keys == 0) + if (n_keys == 0) return; for (i = 0; i < n_keys; i++) { @@ -1503,7 +1506,7 @@ tfilter_parser (KeyfileReaderInfo *info, NMSetting *setting, const char *key) tfilters = g_ptr_array_new_with_free_func ((GDestroyNotify) nm_tc_tfilter_unref); keys = nm_keyfile_plugin_kf_get_keys (info->keyfile, setting_name, &n_keys, NULL); - if (!keys || n_keys == 0) + if (n_keys == 0) return; for (i = 0; i < n_keys; i++) { @@ -1663,16 +1666,21 @@ write_ip_values (GKeyFile *file, const char *gateway, gboolean is_route) { - GString *output; - int family, i; - const char *addr, *gw; + nm_auto_free_gstring GString *output = NULL; + int addr_family; + guint i; + const char *addr; + const char *gw; guint32 plen; - char key_name[64], *key_name_idx; + char key_name[64]; + char *key_name_idx; if (!array->len) return; - family = !strcmp (setting_name, NM_SETTING_IP4_CONFIG_SETTING_NAME) ? AF_INET : AF_INET6; + addr_family = nm_streq (setting_name, NM_SETTING_IP4_CONFIG_SETTING_NAME) + ? AF_INET + : AF_INET6; strcpy (key_name, is_route ? "route" : "address"); key_name_idx = key_name + strlen (key_name); @@ -1693,7 +1701,9 @@ write_ip_values (GKeyFile *file, addr = nm_ip_address_get_address (address); plen = nm_ip_address_get_prefix (address); - gw = i == 0 ? gateway : NULL; + gw = (i == 0) + ? gateway + : NULL; } g_string_set_size (output, 0); @@ -1706,18 +1716,19 @@ write_ip_values (GKeyFile *file, * The current version supports reading of the above form. */ if (!gw) { - if (family == AF_INET) + if (addr_family == AF_INET) gw = "0.0.0.0"; else gw = "::"; } g_string_append_printf (output, ",%s", gw); - if (is_route && metric != -1) + if ( is_route + && metric != -1) g_string_append_printf (output, ",%lu", (unsigned long) metric); } - sprintf (key_name_idx, "%d", i + 1); + sprintf (key_name_idx, "%u", i + 1); nm_keyfile_plugin_kf_set_string (file, setting_name, key_name, output->str); if (is_route) { @@ -1732,7 +1743,6 @@ write_ip_values (GKeyFile *file, } } } - g_string_free (output, TRUE); } static void @@ -1843,7 +1853,8 @@ write_hash_of_string (GKeyFile *file, guint i, l; /* Write VPN secrets out to a different group to keep them separate */ - if (NM_IS_SETTING_VPN (setting) && !strcmp (key, NM_SETTING_VPN_SECRETS)) { + if ( NM_IS_SETTING_VPN (setting) + && nm_streq (key, NM_SETTING_VPN_SECRETS)) { group_name = NM_KEYFILE_GROUP_VPN_SECRETS; vpn_secrets = TRUE; } @@ -1976,9 +1987,10 @@ cert_writer_default (NMConnection *connection, scheme = cert_data->vtable->scheme_func (cert_data->setting); if (scheme == NM_SETTING_802_1X_CK_SCHEME_PATH) { - const char *path; - char *path_free = NULL, *tmp; + gs_free char *path_free = NULL; gs_free char *base_dir = NULL; + gs_free char *tmp = NULL; + const char *path; path = cert_data->vtable->path_func (cert_data->setting); g_assert (path); @@ -1988,7 +2000,8 @@ cert_writer_default (NMConnection *connection, * context. */ if (path[0] && path[0] != '/') { base_dir = g_get_current_dir (); - path = path_free = g_strconcat (base_dir, "/", path, NULL); + path_free = g_strconcat (base_dir, "/", path, NULL); + path = path_free; } else base_dir = g_path_get_dirname (path); @@ -1998,20 +2011,21 @@ cert_writer_default (NMConnection *connection, tmp = nm_keyfile_detect_unqualified_path_scheme (base_dir, path, -1, FALSE, NULL); if (tmp) g_clear_pointer (&tmp, g_free); - else - path = tmp = g_strconcat (NM_KEYFILE_CERT_SCHEME_PREFIX_PATH, path, NULL); + else { + tmp = g_strconcat (NM_KEYFILE_CERT_SCHEME_PREFIX_PATH, path, NULL); + path = tmp; + } /* Path contains at least a '/', hence it cannot be recognized as the old * binary format consisting of a list of integers. */ nm_keyfile_plugin_kf_set_string (file, setting_name, cert_data->vtable->setting_key, path); - g_free (tmp); - g_free (path_free); } else if (scheme == NM_SETTING_802_1X_CK_SCHEME_BLOB) { GBytes *blob; const guint8 *blob_data; gsize blob_len; - char *blob_base64, *val; + gs_free char *blob_base64 = NULL; + gs_free char *val = NULL; blob = cert_data->vtable->blob_func (cert_data->setting); g_assert (blob); @@ -2021,8 +2035,6 @@ cert_writer_default (NMConnection *connection, val = g_strconcat (NM_KEYFILE_CERT_SCHEME_PREFIX_BLOB, blob_base64, NULL); nm_keyfile_plugin_kf_set_string (file, setting_name, cert_data->vtable->setting_key, val); - g_free (val); - g_free (blob_base64); } else if (scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11) { nm_keyfile_plugin_kf_set_string (file, setting_name, cert_data->vtable->setting_key, cert_data->vtable->uri_func (cert_data->setting)); @@ -2048,7 +2060,7 @@ cert_writer (KeyfileWriterInfo *info, NMKeyfileWriteTypeDataCert type_data = { 0 }; for (i = 0; nm_setting_8021x_scheme_vtable[i].setting_key; i++) { - if (g_strcmp0 (nm_setting_8021x_scheme_vtable[i].setting_key, key) == 0) { + if (nm_streq0 (nm_setting_8021x_scheme_vtable[i].setting_key, key)) { objtype = &nm_setting_8021x_scheme_vtable[i]; break; } @@ -2621,7 +2633,7 @@ read_one_setting_value (NMSetting *setting, } else g_object_set (setting, key, int_val, NULL); } else if (type == G_TYPE_BYTES) { - int *tmp; + gs_free int *tmp = NULL; GByteArray *array; GBytes *bytes; gsize length; @@ -2632,7 +2644,7 @@ read_one_setting_value (NMSetting *setting, array = g_byte_array_sized_new (length); for (i = 0; i < length; i++) { - int val = tmp[i]; + const int val = tmp[i]; unsigned char v = (unsigned char) (val & 0xFF); if (val < 0 || val > 255) { @@ -2641,7 +2653,6 @@ read_one_setting_value (NMSetting *setting, _("ignoring invalid byte element '%d' (not between 0 and 255 inclusive)"), val)) { g_byte_array_unref (array); - g_free (tmp); return; } already_warned = TRUE; @@ -2652,14 +2663,12 @@ read_one_setting_value (NMSetting *setting, bytes = g_byte_array_free_to_bytes (array); g_object_set (setting, key, bytes, NULL); g_bytes_unref (bytes); - g_free (tmp); } else if (type == G_TYPE_STRV) { - char **sa; + gs_strfreev char **sa = NULL; gsize length; sa = nm_keyfile_plugin_kf_get_string_list (keyfile, setting_name, key, &length, NULL); g_object_set (setting, key, sa, NULL); - g_strfreev (sa); } else if (type == G_TYPE_HASH_TABLE) { read_hash_of_string (keyfile, setting, key); } else if (type == G_TYPE_ARRAY) { @@ -2693,8 +2702,8 @@ read_one_setting_value (NMSetting *setting, } } -static NMSetting * -read_setting (KeyfileReaderInfo *info) +static void +_read_setting (KeyfileReaderInfo *info) { const NMSettInfoSetting *sett_info; gs_unref_object NMSetting *setting = NULL; @@ -2709,7 +2718,7 @@ read_setting (KeyfileReaderInfo *info) if (!type) { handle_warn (info, NULL, NM_KEYFILE_WARN_SEVERITY_WARN, _("invalid setting name '%s'"), info->group); - return NULL; + return; } setting = g_object_new (type, NULL); @@ -2735,7 +2744,7 @@ read_setting (KeyfileReaderInfo *info) const GVariantType *variant_type; GVariant *variant; - /* a GKeyfile can return duplicate keys, there is just no API to make sense + /* a GKeyFile can return duplicate keys, there is just no API to make sense * of them. Skip them. */ if ( i + 1 < n_keys && nm_streq (key, keys[i + 1])) @@ -2786,31 +2795,39 @@ read_setting (KeyfileReaderInfo *info) for (; i < n_keys; i++) g_free (keys[i]); } - } else - nm_setting_enumerate_values (setting, read_one_setting_value, info); + goto out; + } + nm_setting_enumerate_values (setting, read_one_setting_value, info); + +out: info->setting = NULL; - - if (info->error) - return NULL; - return g_steal_pointer (&setting); + if (!info->error) + nm_connection_add_setting (info->connection, g_steal_pointer (&setting)); } static void -read_vpn_secrets (KeyfileReaderInfo *info, NMSettingVpn *s_vpn) +_read_setting_vpn_secrets (KeyfileReaderInfo *info) { gs_strfreev char **keys = NULL; gsize i, n_keys; + NMSettingVpn *s_vpn; + + s_vpn = nm_connection_get_setting_vpn (info->connection); + if (!s_vpn) { + /* if we don't also have a [vpn] section (which must be parsed earlier), + * we don't do anything. */ + nm_assert (!g_key_file_has_group (info->keyfile, "vpn")); + return; + } keys = nm_keyfile_plugin_kf_get_keys (info->keyfile, NM_KEYFILE_GROUP_VPN_SECRETS, &n_keys, NULL); for (i = 0; i < n_keys; i++) { - char *secret; + gs_free char *secret = NULL; secret = nm_keyfile_plugin_kf_get_string (info->keyfile, NM_KEYFILE_GROUP_VPN_SECRETS, keys[i], NULL); - if (secret) { + if (secret) nm_setting_vpn_add_secret (s_vpn, keys[i], secret); - g_free (secret); - } } } @@ -2878,12 +2895,11 @@ nm_keyfile_read (GKeyFile *keyfile, { gs_unref_object NMConnection *connection = NULL; NMSettingConnection *s_con; - NMSetting *setting; gs_strfreev char **groups = NULL; - gsize length; + gsize n_groups; gsize i; gboolean vpn_secrets = FALSE; - KeyfileReaderInfo info = { 0 }; + KeyfileReaderInfo info; g_return_val_if_fail (keyfile, NULL); g_return_val_if_fail (!error || !*error, NULL); @@ -2891,32 +2907,32 @@ nm_keyfile_read (GKeyFile *keyfile, connection = nm_simple_connection_new (); - info.connection = connection; - info.keyfile = (GKeyFile *) keyfile; - info.base_dir = base_dir; - info.handler = handler; - info.user_data = user_data; + info = (KeyfileReaderInfo) { + .connection = connection, + .keyfile = keyfile, + .base_dir = base_dir, + .handler = handler, + .user_data = user_data, + }; - groups = g_key_file_get_groups (keyfile, &length); + groups = g_key_file_get_groups (keyfile, &n_groups); if (!groups) - length = 0; + n_groups = 0; - for (i = 0; i < length; i++) { - /* Only read out secrets when needed */ - if (!strcmp (groups[i], NM_KEYFILE_GROUP_VPN_SECRETS)) { - vpn_secrets = TRUE; - continue; - } + for (i = 0; i < n_groups; i++) { info.group = groups[i]; - setting = read_setting (&info); + + if (nm_streq (groups[i], NM_KEYFILE_GROUP_VPN_SECRETS)) { + /* Only read out secrets when needed */ + vpn_secrets = TRUE; + } else + _read_setting (&info); + info.group = NULL; - if (info.error) { - g_propagate_error (error, info.error); - return NULL; - } - if (setting) - nm_connection_add_setting (connection, setting); + + if (info.error) + goto out_with_info_error; } s_con = nm_connection_get_setting_connection (connection); @@ -2930,33 +2946,29 @@ nm_keyfile_read (GKeyFile *keyfile, */ if ( !nm_setting_connection_get_interface_name (s_con) && nm_setting_connection_get_connection_type (s_con)) { - char *interface_name; + gs_free char *interface_name = NULL; interface_name = g_key_file_get_string (keyfile, nm_setting_connection_get_connection_type (s_con), "interface-name", NULL); - if (interface_name) { + if (interface_name) g_object_set (s_con, NM_SETTING_CONNECTION_INTERFACE_NAME, interface_name, NULL); - g_free (interface_name); - } } - /* Handle vpn secrets after the 'vpn' setting was read */ if (vpn_secrets) { - NMSettingVpn *s_vpn; - - s_vpn = nm_connection_get_setting_vpn (connection); - if (s_vpn) { - read_vpn_secrets (&info, s_vpn); - if (info.error) { - g_propagate_error (error, info.error); - return NULL; - } - } + info.group = NM_KEYFILE_GROUP_VPN_SECRETS; + _read_setting_vpn_secrets (&info); + info.group = NULL;; + if (info.error) + goto out_with_info_error; } return g_steal_pointer (&connection); + +out_with_info_error: + g_propagate_error (error, info.error); + return NULL; } /*****************************************************************************/ @@ -3086,9 +3098,10 @@ nm_keyfile_write (NMConnection *connection, void *user_data, GError **error) { - KeyfileWriterInfo info = { 0 }; + gs_unref_keyfile GKeyFile *keyfile = NULL; + KeyfileWriterInfo info; gs_free NMSetting **settings = NULL; - guint i, length = 0; + guint i, n_settings = 0; g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL); g_return_val_if_fail (!error || !*error, NULL); @@ -3096,14 +3109,18 @@ nm_keyfile_write (NMConnection *connection, if (!nm_connection_verify (connection, error)) return NULL; - info.connection = connection; - info.keyfile = g_key_file_new (); - info.error = NULL; - info.handler = handler; - info.user_data = user_data; + keyfile = g_key_file_new (); - settings = nm_connection_get_settings (connection, &length); - for (i = 0; i < length; i++) { + info = (KeyfileWriterInfo) { + .connection = connection, + .keyfile = keyfile, + .error = NULL, + .handler = handler, + .user_data = user_data, + }; + + settings = nm_connection_get_settings (connection, &n_settings); + for (i = 0; i < n_settings; i++) { const NMSettInfoSetting *sett_info; NMSetting *setting = settings[i]; @@ -3142,20 +3159,24 @@ nm_keyfile_write (NMConnection *connection, } } } - } else - nm_setting_enumerate_values (setting, write_setting_value, &info); + goto next; + } + nm_setting_enumerate_values (setting, write_setting_value, &info); + +next: if (info.error) - break; + goto out_with_info_error; } - if (info.error) { - g_propagate_error (error, info.error); - g_key_file_unref (info.keyfile); - return NULL; - } + if (info.error) + goto out_with_info_error; - return info.keyfile; + return g_steal_pointer (&keyfile); + +out_with_info_error: + g_propagate_error (error, info.error); + return NULL; } /*****************************************************************************/ diff --git a/shared/nm-utils/nm-shared-utils.c b/shared/nm-utils/nm-shared-utils.c index 87094a30df..9b020429d6 100644 --- a/shared/nm-utils/nm-shared-utils.c +++ b/shared/nm-utils/nm-shared-utils.c @@ -1196,7 +1196,7 @@ nm_g_object_set_property_boolean (GObject *object, gboolean nm_g_object_set_property_uint (GObject *object, - const char *property_name, + const char *property_name, guint value, GError **error) {