From ce6fdc3e5e66a17b4fdc4519500aaa06272bce5c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 17 Mar 2016 11:39:08 +0100 Subject: [PATCH 01/28] libnm-util: verify connection also for self-assignment in replace_settings_from_connection() nm_connection_replace_settings_from_connection() would return whether the connection verifies at the end of the operation. While that is not very useful, the API is like that and cannot be changed. For consistency, also perform the verification step in case of self-assignment. Self-assigment is anyway a case that probably never happens. --- libnm-util/nm-connection.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/libnm-util/nm-connection.c b/libnm-util/nm-connection.c index c440fb1b28..daeee46b7e 100644 --- a/libnm-util/nm-connection.c +++ b/libnm-util/nm-connection.c @@ -409,7 +409,8 @@ nm_connection_replace_settings_from_connection (NMConnection *connection, NMConnectionPrivate *priv; GHashTableIter iter; NMSetting *setting; - gboolean changed, valid; + gboolean changed = FALSE; + gboolean valid; g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE); g_return_val_if_fail (NM_IS_CONNECTION (new_connection), FALSE); @@ -418,7 +419,7 @@ nm_connection_replace_settings_from_connection (NMConnection *connection, /* When 'connection' and 'new_connection' are the same object simply return * in order not to destroy 'connection' */ if (connection == new_connection) - return TRUE; + goto out; /* No need to validate permissions like nm_connection_replace_settings() * since we're dealing with an NMConnection which has already done that. @@ -435,6 +436,7 @@ nm_connection_replace_settings_from_connection (NMConnection *connection, changed = TRUE; } +out: valid = nm_connection_verify (connection, error); if (changed) g_signal_emit (connection, signals[CHANGED], 0); From d45107c1ded71ad9138b7158be5454c88062e449 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 Mar 2016 19:52:48 +0100 Subject: [PATCH 02/28] nmtst: fix memleak when using NMTST_VARIANT_DROP_SETTING() --- shared/nm-test-utils.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/shared/nm-test-utils.h b/shared/nm-test-utils.h index a7d0f93cda..55c9101c94 100644 --- a/shared/nm-test-utils.h +++ b/shared/nm-test-utils.h @@ -1863,6 +1863,8 @@ typedef enum { \ if (__cur_setting_name) \ g_variant_builder_add (&__connection_builder, "{sa{sv}}", __cur_setting_name, &__setting_builder); \ + else \ + g_variant_builder_clear (&__setting_builder); \ g_variant_iter_free (__setting_iter); \ } \ \ From 2c8ef153a1dd6ef7b0a4f1a0ffa890a7a8e2b6bd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 17 Mar 2016 12:42:54 +0100 Subject: [PATCH 03/28] nmtst: add nmtst_variant_new_vardict() function --- shared/nm-test-utils.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/shared/nm-test-utils.h b/shared/nm-test-utils.h index 55c9101c94..2503245761 100644 --- a/shared/nm-test-utils.h +++ b/shared/nm-test-utils.h @@ -1793,6 +1793,27 @@ nmtst_create_connection_from_keyfile (const char *keyfile_str, const char *keyfi #ifdef __NM_CONNECTION_H__ +inline static GVariant * +_nmtst_variant_new_vardict (int dummy, ...) +{ + GVariantBuilder builder; + va_list ap; + const char *name; + GVariant *variant; + + g_variant_builder_init (&builder, G_VARIANT_TYPE_VARDICT); + + va_start (ap, dummy); + while ((name = va_arg (ap, const char *))) { + variant = va_arg (ap, GVariant *); + g_variant_builder_add (&builder, "{sv}", name, variant); + } + va_end (ap); + + return g_variant_builder_end (&builder); +} +#define nmtst_variant_new_vardict(...) _nmtst_variant_new_vardict (0, __VA_ARGS__, NULL) + #define nmtst_assert_variant_is_of_type(variant, type) \ G_STMT_START { \ GVariant *_variantx = (variant); \ From 3025bfc8b277992dcdd25347c7094ed8a06c32a9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 Mar 2016 16:23:49 +0100 Subject: [PATCH 04/28] shared: add nm_auto_unset_gvalue macro --- shared/nm-macros-internal.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/shared/nm-macros-internal.h b/shared/nm-macros-internal.h index 152cbf67ee..4176231800 100644 --- a/shared/nm-macros-internal.h +++ b/shared/nm-macros-internal.h @@ -36,6 +36,13 @@ #define nm_auto_free nm_auto(_nm_auto_free_impl) GS_DEFINE_CLEANUP_FUNCTION(void*, _nm_auto_free_impl, free) +static inline void +_nm_auto_unset_gvalue_impl (GValue *v) +{ + g_value_unset (v); +} +#define nm_auto_unset_gvalue nm_auto(_nm_auto_unset_gvalue_impl) + /********************************************************/ /* http://stackoverflow.com/a/11172679 */ From 330026db90df99ffc394acc5c08c61e9a3de287a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 Mar 2016 13:45:20 +0100 Subject: [PATCH 05/28] shared: add "nm-shared-utils" --- shared/Makefile.am | 2 ++ shared/nm-shared-utils.c | 25 +++++++++++++++++++++++++ shared/nm-shared-utils.h | 27 +++++++++++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 shared/nm-shared-utils.c create mode 100644 shared/nm-shared-utils.h diff --git a/shared/Makefile.am b/shared/Makefile.am index 03fad123fd..c042fdbe80 100644 --- a/shared/Makefile.am +++ b/shared/Makefile.am @@ -4,6 +4,8 @@ EXTRA_DIST = \ nm-default.h \ nm-glib.h \ nm-macros-internal.h \ + nm-shared-utils.c \ + nm-shared-utils.h \ nm-test-libnm-utils.h \ nm-test-utils.h \ nm-test-utils-impl.c \ diff --git a/shared/nm-shared-utils.c b/shared/nm-shared-utils.c new file mode 100644 index 0000000000..b2c164e19a --- /dev/null +++ b/shared/nm-shared-utils.c @@ -0,0 +1,25 @@ +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */ +/* NetworkManager -- Network link manager + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the + * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, + * Boston, MA 02110-1301 USA. + * + * (C) Copyright 2016 Red Hat, Inc. + */ + +#include "nm-default.h" + +#include "nm-shared-utils.h" + diff --git a/shared/nm-shared-utils.h b/shared/nm-shared-utils.h new file mode 100644 index 0000000000..2619d04978 --- /dev/null +++ b/shared/nm-shared-utils.h @@ -0,0 +1,27 @@ +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */ +/* NetworkManager -- Network link manager + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the + * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, + * Boston, MA 02110-1301 USA. + * + * (C) Copyright 2016 Red Hat, Inc. + */ + +#ifndef __NM_SHARED_UTILS_H__ +#define __NM_SHARED_UTILS_H__ + +/******************************************************************************/ + +#endif /* __NM_SHARED_UTILS_H__ */ From 5de30dd0296087b5bbfac3cd44cc325a31ceab14 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 Mar 2016 14:08:41 +0100 Subject: [PATCH 06/28] shared: include "nm-shared-utils.h" Include it via "nm-default.h" to all projects. --- libnm-core/Makefile.libnm-core | 2 ++ shared/nm-default.h | 1 + 2 files changed, 3 insertions(+) diff --git a/libnm-core/Makefile.libnm-core b/libnm-core/Makefile.libnm-core index fe8cc8a603..0038ce23d6 100644 --- a/libnm-core/Makefile.libnm-core +++ b/libnm-core/Makefile.libnm-core @@ -52,6 +52,7 @@ libnm_core_headers = \ $(core)/nm-vpn-plugin-info.h libnm_core_private_headers = \ + $(top_builddir)/shared/nm-shared-utils.h \ $(core)/crypto.h \ $(core)/nm-connection-private.h \ $(core)/nm-core-internal.h \ @@ -63,6 +64,7 @@ libnm_core_private_headers = \ $(core)/nm-utils-private.h libnm_core_sources = \ + $(top_builddir)/shared/nm-shared-utils.c \ $(core_build)/nm-core-enum-types.c \ $(core)/crypto.c \ $(core)/nm-connection.c \ diff --git a/shared/nm-default.h b/shared/nm-default.h index 365277724d..aae4777a2f 100644 --- a/shared/nm-default.h +++ b/shared/nm-default.h @@ -52,6 +52,7 @@ #include "nm-version.h" #include "gsystem-local-alloc.h" #include "nm-macros-internal.h" +#include "nm-shared-utils.h" /*****************************************************************************/ From fafc90526be534a9cc4acdbcd02df43424d8304b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 20 Mar 2016 10:32:43 +0100 Subject: [PATCH 07/28] shared: move _nm_utils_ascii_str_to_int64() to "shared/nm-shared-utils.h" _nm_utils_ascii_str_to_int64() was declared in libnm-core's internal header "nm-core-internal.h" and thus available for libnm-core, libnm, NetworkManager and related. It also means, the function was not available in libnm-util, libnm-glib, clients or dispatcher. So, we either reimplemented it (nmc_string_to_int_base) or struggle with the awkward strtol* API. --- libnm-core/nm-core-internal.h | 2 - libnm-core/nm-utils.c | 75 --------------------------------- shared/nm-shared-utils.c | 78 +++++++++++++++++++++++++++++++++++ shared/nm-shared-utils.h | 4 ++ 4 files changed, 82 insertions(+), 77 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 3e1236b975..92de9d1313 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -179,8 +179,6 @@ GByteArray *nm_utils_rsa_key_encrypt (const guint8 *data, char **out_password, GError **error); -gint64 _nm_utils_ascii_str_to_int64 (const char *str, guint base, gint64 min, gint64 max, gint64 fallback); - gulong _nm_dbus_signal_connect_data (GDBusProxy *proxy, const char *signal_name, const GVariantType *signature, diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 10d54fa86b..a846708101 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -3789,81 +3789,6 @@ _nm_utils_strstrdictkey_create (const char *v1, const char *v2) return k; } -/**********************************************************************************************/ - -/* _nm_utils_ascii_str_to_int64: - * - * A wrapper for g_ascii_strtoll, that checks whether the whole string - * can be successfully converted to a number and is within a given - * range. On any error, @fallback will be returned and %errno will be set - * to a non-zero value. On success, %errno will be set to zero, check %errno - * for errors. Any trailing or leading (ascii) white space is ignored and the - * functions is locale independent. - * - * The function is guaranteed to return a value between @min and @max - * (inclusive) or @fallback. Also, the parsing is rather strict, it does - * not allow for any unrecognized characters, except leading and trailing - * white space. - **/ -gint64 -_nm_utils_ascii_str_to_int64 (const char *str, guint base, gint64 min, gint64 max, gint64 fallback) -{ - gint64 v; - size_t len; - char buf[64], *s, *str_free = NULL; - - if (str) { - while (g_ascii_isspace (str[0])) - str++; - } - if (!str || !str[0]) { - errno = EINVAL; - return fallback; - } - - len = strlen (str); - if (g_ascii_isspace (str[--len])) { - /* backward search the first non-ws character. - * We already know that str[0] is non-ws. */ - while (g_ascii_isspace (str[--len])) - ; - - /* str[len] is now the last non-ws character... */ - len++; - - if (len >= sizeof (buf)) - s = str_free = g_malloc (len + 1); - else - s = buf; - - memcpy (s, str, len); - s[len] = 0; - - nm_assert (len > 0 && len < strlen (str) && len == strlen (s)); - nm_assert (!g_ascii_isspace (str[len-1]) && g_ascii_isspace (str[len])); - nm_assert (strncmp (str, s, len) == 0); - - str = s; - } - - errno = 0; - v = g_ascii_strtoll (str, &s, base); - - if (errno != 0) - v = fallback; - else if (s[0] != 0) { - errno = EINVAL; - v = fallback; - } else if (v > max || v < min) { - errno = ERANGE; - v = fallback; - } - - if (G_UNLIKELY (str_free)) - g_free (str_free); - return v; -} - static gboolean validate_dns_option (const char *name, gboolean numeric, gboolean ipv6, const NMUtilsDNSOptionDesc *option_descs) diff --git a/shared/nm-shared-utils.c b/shared/nm-shared-utils.c index b2c164e19a..b80b9f2aba 100644 --- a/shared/nm-shared-utils.c +++ b/shared/nm-shared-utils.c @@ -23,3 +23,81 @@ #include "nm-shared-utils.h" +#include + +/*****************************************************************************/ + +/* _nm_utils_ascii_str_to_int64: + * + * A wrapper for g_ascii_strtoll, that checks whether the whole string + * can be successfully converted to a number and is within a given + * range. On any error, @fallback will be returned and %errno will be set + * to a non-zero value. On success, %errno will be set to zero, check %errno + * for errors. Any trailing or leading (ascii) white space is ignored and the + * functions is locale independent. + * + * The function is guaranteed to return a value between @min and @max + * (inclusive) or @fallback. Also, the parsing is rather strict, it does + * not allow for any unrecognized characters, except leading and trailing + * white space. + **/ +gint64 +_nm_utils_ascii_str_to_int64 (const char *str, guint base, gint64 min, gint64 max, gint64 fallback) +{ + gint64 v; + size_t len; + char buf[64], *s, *str_free = NULL; + + if (str) { + while (g_ascii_isspace (str[0])) + str++; + } + if (!str || !str[0]) { + errno = EINVAL; + return fallback; + } + + len = strlen (str); + if (g_ascii_isspace (str[--len])) { + /* backward search the first non-ws character. + * We already know that str[0] is non-ws. */ + while (g_ascii_isspace (str[--len])) + ; + + /* str[len] is now the last non-ws character... */ + len++; + + if (len >= sizeof (buf)) + s = str_free = g_malloc (len + 1); + else + s = buf; + + memcpy (s, str, len); + s[len] = 0; + + nm_assert (len > 0 && len < strlen (str) && len == strlen (s)); + nm_assert (!g_ascii_isspace (str[len-1]) && g_ascii_isspace (str[len])); + nm_assert (strncmp (str, s, len) == 0); + + str = s; + } + + errno = 0; + v = g_ascii_strtoll (str, &s, base); + + if (errno != 0) + v = fallback; + else if (s[0] != 0) { + errno = EINVAL; + v = fallback; + } else if (v > max || v < min) { + errno = ERANGE; + v = fallback; + } + + if (G_UNLIKELY (str_free)) + g_free (str_free); + return v; +} + +/*****************************************************************************/ diff --git a/shared/nm-shared-utils.h b/shared/nm-shared-utils.h index 2619d04978..943e467d82 100644 --- a/shared/nm-shared-utils.h +++ b/shared/nm-shared-utils.h @@ -24,4 +24,8 @@ /******************************************************************************/ +gint64 _nm_utils_ascii_str_to_int64 (const char *str, guint base, gint64 min, gint64 max, gint64 fallback); + +/******************************************************************************/ + #endif /* __NM_SHARED_UTILS_H__ */ From 72216f73599760977a54bb8b96b1c8054f92ba6a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 Mar 2016 13:59:57 +0100 Subject: [PATCH 08/28] shared: move NM_UTILS_ERROR to shared-utils NM_UTILS_ERROR is our way to say, that we don't care about the GError domain and code. nmcli sometimes passes domain "1" and code "0" to g_set_error(), which could be considered a bug. We usually don't care about the error but only about the error message, so let's have a universally available error quark around. --- shared/nm-shared-utils.c | 33 +++++++++++++++++++++++++++++++++ shared/nm-shared-utils.h | 25 +++++++++++++++++++++++++ src/nm-core-utils.c | 33 --------------------------------- src/nm-core-utils.h | 25 ------------------------- 4 files changed, 58 insertions(+), 58 deletions(-) diff --git a/shared/nm-shared-utils.c b/shared/nm-shared-utils.c index b80b9f2aba..57b979bc22 100644 --- a/shared/nm-shared-utils.c +++ b/shared/nm-shared-utils.c @@ -101,3 +101,36 @@ _nm_utils_ascii_str_to_int64 (const char *str, guint base, gint64 min, gint64 ma } /*****************************************************************************/ + +G_DEFINE_QUARK (nm-utils-error-quark, nm_utils_error) + +void +nm_utils_error_set_cancelled (GError **error, + gboolean is_disposing, + const char *instance_name) +{ + if (is_disposing) { + g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_CANCELLED_DISPOSING, + "Disposing %s instance", + instance_name && *instance_name ? instance_name : "source"); + } else { + g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_CANCELLED, + "Request cancelled"); + } +} + +gboolean +nm_utils_error_is_cancelled (GError *error, + gboolean consider_is_disposing) +{ + if (error) { + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + return TRUE; + if ( consider_is_disposing + && g_error_matches (error, NM_UTILS_ERROR, NM_UTILS_ERROR_CANCELLED_DISPOSING)) + return TRUE; + } + return FALSE; +} + +/*****************************************************************************/ diff --git a/shared/nm-shared-utils.h b/shared/nm-shared-utils.h index 943e467d82..2942fe9bb6 100644 --- a/shared/nm-shared-utils.h +++ b/shared/nm-shared-utils.h @@ -28,4 +28,29 @@ gint64 _nm_utils_ascii_str_to_int64 (const char *str, guint base, gint64 min, gi /******************************************************************************/ +/** + * NMUtilsError: + * @NM_UTILS_ERROR_UNKNOWN: unknown or unclassified error + * @NM_UTILS_ERROR_CANCELLED_DISPOSING: when disposing an object that has + * pending aynchronous operations, the operation is cancelled with this + * error reason. Depending on the usage, this might indicate a bug because + * usually the target object should stay alive as long as there are pending + * operations. + */ +typedef enum { + NM_UTILS_ERROR_UNKNOWN = 0, /*< nick=Unknown >*/ + NM_UTILS_ERROR_CANCELLED_DISPOSING, /*< nick=CancelledDisposing >*/ +} NMUtilsError; + +#define NM_UTILS_ERROR (nm_utils_error_quark ()) +GQuark nm_utils_error_quark (void); + +void nm_utils_error_set_cancelled (GError **error, + gboolean is_disposing, + const char *instance_name); +gboolean nm_utils_error_is_cancelled (GError *error, + gboolean consider_is_disposing); + +/******************************************************************************/ + #endif /* __NM_SHARED_UTILS_H__ */ diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 1a6d00f973..b98b6e8abd 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -109,39 +109,6 @@ _nm_utils_set_testing (NMUtilsTestFlags flags) /*****************************************************************************/ -G_DEFINE_QUARK (nm-utils-error-quark, nm_utils_error) - -void -nm_utils_error_set_cancelled (GError **error, - gboolean is_disposing, - const char *instance_name) -{ - if (is_disposing) { - g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_CANCELLED_DISPOSING, - "Disposing %s instance", - instance_name && *instance_name ? instance_name : "source"); - } else { - g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_CANCELLED, - "Request cancelled"); - } -} - -gboolean -nm_utils_error_is_cancelled (GError *error, - gboolean consider_is_disposing) -{ - if (error) { - if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) - return TRUE; - if ( consider_is_disposing - && g_error_matches (error, NM_UTILS_ERROR, NM_UTILS_ERROR_CANCELLED_DISPOSING)) - return TRUE; - } - return FALSE; -} - -/*****************************************************************************/ - static GSList *_singletons = NULL; static gboolean _singletons_shutdown = FALSE; diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index 2fb5273d74..f77b43e2a3 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -91,31 +91,6 @@ GETTER (void) \ /*****************************************************************************/ -/** - * NMUtilsError: - * @NM_UTILS_ERROR_UNKNOWN: unknown or unclassified error - * @NM_UTILS_ERROR_CANCELLED_DISPOSING: when disposing an object that has - * pending aynchronous operations, the operation is cancelled with this - * error reason. Depending on the usage, this might indicate a bug because - * usually the target object should stay alive as long as there are pending - * operations. - */ -typedef enum { - NM_UTILS_ERROR_UNKNOWN = 0, /*< nick=Unknown >*/ - NM_UTILS_ERROR_CANCELLED_DISPOSING, /*< nick=CancelledDisposing >*/ -} NMUtilsError; - -#define NM_UTILS_ERROR (nm_utils_error_quark ()) -GQuark nm_utils_error_quark (void); - -void nm_utils_error_set_cancelled (GError **error, - gboolean is_disposing, - const char *instance_name); -gboolean nm_utils_error_is_cancelled (GError *error, - gboolean consider_is_disposing); - -/*****************************************************************************/ - gint nm_utils_ascii_str_to_bool (const char *str, gint default_value); From c5786f38398f5ba8086686a53c7898bb19b91546 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 17 Mar 2016 12:19:23 +0100 Subject: [PATCH 09/28] libnm/tests: extend tests for handling invalid connections in NMClient --- libnm/tests/test-nm-client.c | 177 ++++++++++++++++++++++++++++------- 1 file changed, 142 insertions(+), 35 deletions(-) diff --git a/libnm/tests/test-nm-client.c b/libnm/tests/test-nm-client.c index 107cc58871..c6a4a05cab 100644 --- a/libnm/tests/test-nm-client.c +++ b/libnm/tests/test-nm-client.c @@ -1243,10 +1243,12 @@ _test_connection_invalid_find_connections (gpointer element, gpointer needle, gp } #define ASSERT_IDX(i) \ - g_assert_cmpint (idx[i], >=, 0); \ - g_assert (path##i && *path##i); \ - g_assert (NM_IS_REMOTE_CONNECTION (connections->pdata[idx[i]])); \ - g_assert_cmpstr (nm_connection_get_path (connections->pdata[idx[i]]), ==, path##i); + G_STMT_START { \ + g_assert_cmpint (idx[i], >=, 0); \ + g_assert (path##i && *path##i); \ + g_assert (NM_IS_REMOTE_CONNECTION (connections->pdata[idx[i]])); \ + g_assert_cmpstr (nm_connection_get_path (connections->pdata[idx[i]]), ==, path##i); \ + } G_STMT_END static void test_connection_invalid (void) @@ -1260,12 +1262,14 @@ test_connection_invalid (void) gs_free char *path0 = NULL; gs_free char *path1 = NULL; gs_free char *path2 = NULL; + gs_free char *path3 = NULL; gs_free char *uuid2 = NULL; gsize n_found; - gssize idx[3]; + gssize idx[4]; + gs_unref_variant GVariant *variant = NULL; /************************************************************************** - * Add two connection before starting libnm. One valid, one invalid. + * Add three connections before starting libnm. One valid, two invalid. *************************************************************************/ connection = nmtst_create_minimal_connection ("test-connection-invalid-0", NULL, NM_SETTING_WIRED_SETTING_NAME, &s_con); @@ -1289,26 +1293,45 @@ test_connection_invalid (void) FALSE, &path1); + g_object_set (s_con, + NM_SETTING_CONNECTION_ID, "test-connection-invalid-2", + NM_SETTING_CONNECTION_TYPE, "invalid-type-2", + NM_SETTING_CONNECTION_UUID, nmtst_uuid_generate (), + NULL); + variant = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_ALL); + NMTST_VARIANT_EDITOR (variant, + NMTST_VARIANT_ADD_SETTING ("invalid-type-2", + nmtst_variant_new_vardict ("some-key1", g_variant_new_string ("some-value1"), + "some-key2", g_variant_new_uint32 (4722)))); + g_variant_ref_sink (variant); + nmtstc_service_add_connection_variant (my_sinfo, + variant, + FALSE, + &path2); + g_test_expect_message ("libnm", G_LOG_LEVEL_WARNING, "*replace_settings: error updating connection /org/freedesktop/NetworkManager/Settings/Connection/3 settings: *"); client = nm_client_new (NULL, &error); + g_test_assert_expected_messages (); g_assert_no_error (error); connections = nm_client_get_connections (client); g_assert (connections); - g_assert_cmpint (connections->len, ==, 2); + g_assert_cmpint (connections->len, ==, 3); n_found = nmtst_find_all_indexes (connections->pdata, connections->len, - (gpointer *) ((const char *[]) { path0, path1 }), - 2, + (gpointer *) ((const char *[]) { path0, path1, path2 }), + 3, _test_connection_invalid_find_connections, NULL, idx); - g_assert_cmpint (n_found, ==, 2); + g_assert_cmpint (n_found, ==, 3); ASSERT_IDX (0); ASSERT_IDX (1); + ASSERT_IDX (2); nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[0]]); nmtst_assert_connection_unnormalizable (connections->pdata[idx[1]], 0, 0); + nmtst_assert_connection_unnormalizable (connections->pdata[idx[2]], 0, 0); /************************************************************************** * After having the client up and running, add another invalid connection @@ -1322,7 +1345,7 @@ test_connection_invalid (void) nmtstc_service_add_connection (my_sinfo, connection, FALSE, - &path2); + &path3); nmtst_main_loop_run (loop, 100); @@ -1331,21 +1354,99 @@ test_connection_invalid (void) connections = nm_client_get_connections (client); g_assert (connections); - g_assert_cmpint (connections->len, ==, 3); + g_assert_cmpint (connections->len, ==, 4); n_found = nmtst_find_all_indexes (connections->pdata, connections->len, - (gpointer *) ((const char *[]) { path0, path1, path2 }), - 3, + (gpointer *) ((const char *[]) { path0, path1, path2, path3 }), + 4, _test_connection_invalid_find_connections, NULL, idx); - g_assert_cmpint (n_found, ==, 3); + g_assert_cmpint (n_found, ==, 4); ASSERT_IDX (0); ASSERT_IDX (1); ASSERT_IDX (2); + ASSERT_IDX (3); nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[0]]); nmtst_assert_connection_unnormalizable (connections->pdata[idx[1]], 0, 0); nmtst_assert_connection_unnormalizable (connections->pdata[idx[2]], 0, 0); + nmtst_assert_connection_unnormalizable (connections->pdata[idx[3]], 0, 0); + + /************************************************************************** + * Modify the invalid connection (still invalid) + *************************************************************************/ + + NMTST_VARIANT_EDITOR (variant, + NMTST_VARIANT_CHANGE_PROPERTY ("invalid-type-2", + "some-key2", "u", 4721)); + g_variant_ref_sink (variant); + nmtstc_service_update_connection_variant (my_sinfo, + path2, + variant, + FALSE); + + g_test_expect_message ("libnm", G_LOG_LEVEL_WARNING, "*replace_settings: error updating connection /org/freedesktop/NetworkManager/Settings/Connection/3 settings: *"); + nmtst_main_loop_run (loop, 100); + g_test_assert_expected_messages (); + + connections = nm_client_get_connections (client); + g_assert (connections); + + g_assert_cmpint (connections->len, ==, 4); + n_found = nmtst_find_all_indexes (connections->pdata, + connections->len, + (gpointer *) ((const char *[]) { path0, path1, path2, path3 }), + 4, + _test_connection_invalid_find_connections, + NULL, + idx); + g_assert_cmpint (n_found, ==, 4); + ASSERT_IDX (0); + ASSERT_IDX (1); + ASSERT_IDX (2); + ASSERT_IDX (3); + nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[0]]); + nmtst_assert_connection_unnormalizable (connections->pdata[idx[1]], 0, 0); + nmtst_assert_connection_unnormalizable (connections->pdata[idx[2]], 0, 0); + nmtst_assert_connection_unnormalizable (connections->pdata[idx[3]], 0, 0); + + /************************************************************************** + * Modify the invalid connection (becomes valid) + *************************************************************************/ + + NMTST_VARIANT_EDITOR (variant, + NMTST_VARIANT_DROP_SETTING ("invalid-type-2")); + NMTST_VARIANT_EDITOR (variant, + NMTST_VARIANT_CHANGE_PROPERTY (NM_SETTING_CONNECTION_SETTING_NAME, + NM_SETTING_CONNECTION_TYPE, "s", NM_SETTING_WIRED_SETTING_NAME)); + g_variant_ref_sink (variant); + nmtstc_service_update_connection_variant (my_sinfo, + path2, + variant, + FALSE); + + nmtst_main_loop_run (loop, 100); + + connections = nm_client_get_connections (client); + g_assert (connections); + + g_assert_cmpint (connections->len, ==, 4); + n_found = nmtst_find_all_indexes (connections->pdata, + connections->len, + (gpointer *) ((const char *[]) { path0, path1, path2, path3 }), + 4, + _test_connection_invalid_find_connections, + NULL, + idx); + g_assert_cmpint (n_found, ==, 4); + ASSERT_IDX (0); + ASSERT_IDX (1); + ASSERT_IDX (2); + ASSERT_IDX (3); + nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[0]]); + nmtst_assert_connection_unnormalizable (connections->pdata[idx[1]], 0, 0); + nmtst_assert_connection_verifies_after_normalization (connections->pdata[idx[2]], 0, 0); + nmtst_assert_connection_unnormalizable (connections->pdata[idx[3]], 0, 0); /************************************************************************** * Modify the invalid connection (still invalid) @@ -1355,7 +1456,7 @@ test_connection_invalid (void) NM_SETTING_CONNECTION_ID, "test-connection-invalid-2x", NULL); nmtstc_service_update_connection (my_sinfo, - path2, + path3, connection, FALSE); @@ -1364,22 +1465,24 @@ test_connection_invalid (void) connections = nm_client_get_connections (client); g_assert (connections); - g_assert_cmpint (connections->len, ==, 3); + g_assert_cmpint (connections->len, ==, 4); n_found = nmtst_find_all_indexes (connections->pdata, connections->len, - (gpointer *) ((const char *[]) { path0, path1, path2 }), - 3, + (gpointer *) ((const char *[]) { path0, path1, path2, path3 }), + 4, _test_connection_invalid_find_connections, NULL, idx); - g_assert_cmpint (n_found, ==, 3); + g_assert_cmpint (n_found, ==, 4); ASSERT_IDX (0); ASSERT_IDX (1); ASSERT_IDX (2); + ASSERT_IDX (3); nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[0]]); nmtst_assert_connection_unnormalizable (connections->pdata[idx[1]], 0, 0); - nmtst_assert_connection_unnormalizable (connections->pdata[idx[2]], 0, 0); - g_assert_cmpstr ("test-connection-invalid-2x", ==, nm_connection_get_id (connections->pdata[idx[2]])); + nmtst_assert_connection_verifies_after_normalization (connections->pdata[idx[2]], 0, 0); + nmtst_assert_connection_unnormalizable (connections->pdata[idx[3]], 0, 0); + g_assert_cmpstr ("test-connection-invalid-2x", ==, nm_connection_get_id (connections->pdata[idx[3]])); /************************************************************************** * Modify the invalid connection (now becomes valid) @@ -1395,7 +1498,7 @@ test_connection_invalid (void) NULL); nmtstc_service_update_connection (my_sinfo, - path2, + path3, connection, FALSE); @@ -1404,22 +1507,24 @@ test_connection_invalid (void) connections = nm_client_get_connections (client); g_assert (connections); - g_assert_cmpint (connections->len, ==, 3); + g_assert_cmpint (connections->len, ==, 4); n_found = nmtst_find_all_indexes (connections->pdata, connections->len, - (gpointer *) ((const char *[]) { path0, path1, path2 }), - 3, + (gpointer *) ((const char *[]) { path0, path1, path2, path3 }), + 4, _test_connection_invalid_find_connections, NULL, idx); - g_assert_cmpint (n_found, ==, 3); + g_assert_cmpint (n_found, ==, 4); ASSERT_IDX (0); ASSERT_IDX (1); ASSERT_IDX (2); + ASSERT_IDX (3); nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[0]]); nmtst_assert_connection_unnormalizable (connections->pdata[idx[1]], 0, 0); - nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[2]]); - g_assert_cmpstr ("test-connection-invalid-2z", ==, nm_connection_get_id (connections->pdata[idx[2]])); + nmtst_assert_connection_verifies_after_normalization (connections->pdata[idx[2]], 0, 0); + nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[3]]); + g_assert_cmpstr ("test-connection-invalid-2z", ==, nm_connection_get_id (connections->pdata[idx[3]])); /************************************************************************** * Modify the invalid connection and make it valid @@ -1444,23 +1549,25 @@ test_connection_invalid (void) connections = nm_client_get_connections (client); g_assert (connections); - g_assert_cmpint (connections->len, ==, 3); + g_assert_cmpint (connections->len, ==, 4); n_found = nmtst_find_all_indexes (connections->pdata, connections->len, - (gpointer *) ((const char *[]) { path0, path1, path2 }), - 3, + (gpointer *) ((const char *[]) { path0, path1, path2, path3 }), + 4, _test_connection_invalid_find_connections, NULL, idx); - g_assert_cmpint (n_found, ==, 3); + g_assert_cmpint (n_found, ==, 4); ASSERT_IDX (0); ASSERT_IDX (1); ASSERT_IDX (2); + ASSERT_IDX (3); nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[0]]); nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[1]]); - nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[2]]); + nmtst_assert_connection_verifies_after_normalization (connections->pdata[idx[2]], 0, 0); + nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[3]]); g_assert_cmpstr ("test-connection-invalid-1x", ==, nm_connection_get_id (connections->pdata[idx[1]])); - g_assert_cmpstr ("test-connection-invalid-2z", ==, nm_connection_get_id (connections->pdata[idx[2]])); + g_assert_cmpstr ("test-connection-invalid-2z", ==, nm_connection_get_id (connections->pdata[idx[3]])); #undef ASSERT_IDX } From 88655999df765373a4808089e26e441d2bfcae14 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 Mar 2016 13:56:53 +0100 Subject: [PATCH 10/28] shared: add nm_g_object_set_property() --- po/POTFILES.in | 1 + shared/nm-shared-utils.c | 92 ++++++++++++++++++++++++++++++++++++++++ shared/nm-shared-utils.h | 7 +++ 3 files changed, 100 insertions(+) diff --git a/po/POTFILES.in b/po/POTFILES.in index 9c37b7ed89..b247e54817 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -140,6 +140,7 @@ libnm/nm-remote-settings.c libnm/nm-vpn-plugin-old.c libnm/nm-vpn-service-plugin.c policy/org.freedesktop.NetworkManager.policy.in.in +shared/nm-shared-utils.c src/NetworkManagerUtils.c src/main.c src/main-utils.c diff --git a/shared/nm-shared-utils.c b/shared/nm-shared-utils.c index 57b979bc22..0ae54bdcf0 100644 --- a/shared/nm-shared-utils.c +++ b/shared/nm-shared-utils.c @@ -134,3 +134,95 @@ nm_utils_error_is_cancelled (GError *error, } /*****************************************************************************/ + +/** + * nm_g_object_set_property: + * @object: the target object + * @property_name: the property name + * @value: the #GValue to set + * @error: (allow-none): optional error argument + * + * A reimplementation of g_object_set_property(), but instead + * returning an error instead of logging a warning. All g_object_set*() + * versions in glib require you to not pass invalid types or they will + * log a g_warning() -- without reporting an error. We don't want that, + * so we need to hack error checking around it. + * + * Returns: whether the value was successfully set. + */ +gboolean +nm_g_object_set_property (GObject *object, + const gchar *property_name, + const GValue *value, + GError **error) +{ + GParamSpec *pspec; + nm_auto_unset_gvalue GValue tmp_value = G_VALUE_INIT; + GObjectClass *klass; + + g_return_val_if_fail (G_IS_OBJECT (object), FALSE); + g_return_val_if_fail (property_name != NULL, FALSE); + g_return_val_if_fail (G_IS_VALUE (value), FALSE); + g_return_val_if_fail (!error || !*error, FALSE); + + /* g_object_class_find_property() does g_param_spec_get_redirect_target(), + * where we differ from a plain g_object_set_property(). */ + pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (object), property_name); + + if (!pspec) { + g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, + _("object class '%s' has no property named '%s'"), + G_OBJECT_TYPE_NAME (object), + property_name); + return FALSE; + } + if (!(pspec->flags & G_PARAM_WRITABLE)) { + g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, + _("property '%s' of object class '%s' is not writable"), + pspec->name, + G_OBJECT_TYPE_NAME (object)); + return FALSE; + } + if ((pspec->flags & G_PARAM_CONSTRUCT_ONLY)) { + g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, + _("construct property \"%s\" for object '%s' can't be set after construction"), + pspec->name, G_OBJECT_TYPE_NAME (object)); + return FALSE; + } + + klass = g_type_class_peek (pspec->owner_type); + if (klass == NULL) { + g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, + _("'%s::%s' is not a valid property name; '%s' is not a GObject subtype"), + g_type_name (pspec->owner_type), pspec->name, g_type_name (pspec->owner_type)); + return FALSE; + } + + /* provide a copy to work from, convert (if necessary) and validate */ + g_value_init (&tmp_value, pspec->value_type); + if (!g_value_transform (value, &tmp_value)) { + g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, + _("unable to set property '%s' of type '%s' from value of type '%s'"), + pspec->name, + g_type_name (pspec->value_type), + G_VALUE_TYPE_NAME (value)); + return FALSE; + } + if ( g_param_value_validate (pspec, &tmp_value) + && !(pspec->flags & G_PARAM_LAX_VALIDATION)) { + gs_free char *contents = g_strdup_value_contents (value); + + g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, + _("value \"%s\" of type '%s' is invalid or out of range for property '%s' of type '%s'"), + contents, + G_VALUE_TYPE_NAME (value), + pspec->name, + g_type_name (pspec->value_type)); + return FALSE; + } + + g_object_set_property (object, property_name, &tmp_value); + return TRUE; +} + +/*****************************************************************************/ diff --git a/shared/nm-shared-utils.h b/shared/nm-shared-utils.h index 2942fe9bb6..f80c850c69 100644 --- a/shared/nm-shared-utils.h +++ b/shared/nm-shared-utils.h @@ -53,4 +53,11 @@ gboolean nm_utils_error_is_cancelled (GError *error, /******************************************************************************/ +gboolean nm_g_object_set_property (GObject *object, + const gchar *property_name, + const GValue *value, + GError **error); + +/******************************************************************************/ + #endif /* __NM_SHARED_UTILS_H__ */ From 737c8cc532263149f0a6779f542ee54ee22fd6ce Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 Mar 2016 13:42:50 +0100 Subject: [PATCH 11/28] libnm-core: allow strict and relaxed error behavior for _nm_setting_new_from_dbus() 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. --- libnm-core/nm-connection.c | 171 +++++++++++++++++++---------- libnm-core/nm-core-internal.h | 14 +++ libnm-core/nm-setting-connection.c | 15 ++- libnm-core/nm-setting-ip-config.c | 11 +- libnm-core/nm-setting-ip4-config.c | 44 ++++++-- libnm-core/nm-setting-ip6-config.c | 44 ++++++-- libnm-core/nm-setting-private.h | 13 ++- libnm-core/nm-setting-vlan.c | 9 +- libnm-core/nm-setting.c | 130 ++++++++++++++++++---- libnm-core/tests/test-general.c | 10 +- 10 files changed, 336 insertions(+), 125 deletions(-) diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c index 04e601ce8c..f0b2387bac 100644 --- a/libnm-core/nm-connection.c +++ b/libnm-core/nm-connection.c @@ -232,6 +232,118 @@ validate_permissions_type (GVariant *variant, GError **error) return valid; } +/** + * _nm_connection_replace_settings: + * @connection: a #NMConnection + * @new_settings: a #GVariant of type %NM_VARIANT_TYPE_CONNECTION, with the new settings + * @parse_flags: flags. + * @error: location to store error, or %NULL + * + * Replaces @connection's settings with @new_settings (which must be + * syntactically valid, and describe a known type of connection, but does not + * need to result in a connection that passes nm_connection_verify()). + * + * Returns: %TRUE if connection was updated, %FALSE if @new_settings could not + * be deserialized (in which case @connection will be unchanged). + **/ +gboolean +_nm_connection_replace_settings (NMConnection *connection, + GVariant *new_settings, + NMSettingParseFlags parse_flags, + GError **error) +{ + NMConnectionPrivate *priv; + GVariantIter iter; + const char *setting_name; + GVariant *setting_dict; + GSList *settings = NULL, *s; + gboolean changed; + + g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE); + g_return_val_if_fail (g_variant_is_of_type (new_settings, NM_VARIANT_TYPE_CONNECTION), FALSE); + g_return_val_if_fail (error == NULL || *error == NULL, FALSE); + + nm_assert (!NM_FLAGS_ANY (parse_flags, ~NM_SETTING_PARSE_FLAGS_ALL)); + nm_assert (!NM_FLAGS_ALL (parse_flags, NM_SETTING_PARSE_FLAGS_STRICT | NM_SETTING_PARSE_FLAGS_BEST_EFFORT)); + + priv = NM_CONNECTION_GET_PRIVATE (connection); + + if ( !NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_BEST_EFFORT) + && !validate_permissions_type (new_settings, error)) + return FALSE; + + g_variant_iter_init (&iter, new_settings); + while (g_variant_iter_next (&iter, "{&s@a{sv}}", &setting_name, &setting_dict)) { + gs_unref_variant GVariant *setting_dict_free = NULL; + GError *local = NULL; + NMSetting *setting; + GType type; + + setting_dict_free = setting_dict; + + type = nm_setting_lookup_type (setting_name); + if (type == G_TYPE_INVALID) { + if (NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_BEST_EFFORT)) + continue; + g_set_error_literal (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_SETTING, + _("unknown setting name")); + g_prefix_error (error, "%s: ", setting_name); + g_slist_free_full (settings, g_object_unref); + return FALSE; + } + + for (s = settings; s; s = s->next) { + if (G_OBJECT_TYPE (s->data) == type) { + if (NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_STRICT)) { + g_set_error_literal (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_SETTING, + _("duplicate setting name")); + g_prefix_error (error, "%s: ", setting_name); + g_slist_free_full (settings, g_object_unref); + return FALSE; + } + /* last wins. */ + g_object_unref (s->data); + settings = g_slist_delete_link (settings, s); + break; + } + } + + setting = _nm_setting_new_from_dbus (type, setting_dict, new_settings, parse_flags, &local); + + if (!setting) { + if (NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_BEST_EFFORT)) + continue; + g_propagate_error (error, local); + g_slist_free_full (settings, g_object_unref); + return FALSE; + } + + settings = g_slist_prepend (settings, setting); + } + + if (g_hash_table_size (priv->settings) > 0) { + g_hash_table_foreach_remove (priv->settings, _setting_release, connection); + changed = TRUE; + } else + changed = (settings != NULL); + + /* Note: @settings might be empty in which case the connection + * has no NMSetting instances... which is fine, just something + * to be aware of. */ + for (s = settings; s; s = s->next) + _nm_connection_add_setting (connection, s->data); + + g_slist_free (settings); + + if (changed) + g_signal_emit (connection, signals[CHANGED], 0); + return TRUE; +} + /** * nm_connection_replace_settings: * @connection: a #NMConnection @@ -250,64 +362,7 @@ nm_connection_replace_settings (NMConnection *connection, GVariant *new_settings, GError **error) { - NMConnectionPrivate *priv; - GVariantIter iter; - const char *setting_name; - GVariant *setting_dict; - GSList *settings = NULL, *s; - gboolean changed; - - g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE); - g_return_val_if_fail (g_variant_is_of_type (new_settings, NM_VARIANT_TYPE_CONNECTION), FALSE); - g_return_val_if_fail (error == NULL || *error == NULL, FALSE); - - priv = NM_CONNECTION_GET_PRIVATE (connection); - - if (!validate_permissions_type (new_settings, error)) - return FALSE; - - g_variant_iter_init (&iter, new_settings); - while (g_variant_iter_next (&iter, "{&s@a{sv}}", &setting_name, &setting_dict)) { - NMSetting *setting; - GType type; - - type = nm_setting_lookup_type (setting_name); - if (type == G_TYPE_INVALID) { - g_set_error_literal (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_SETTING, - _("unknown setting name")); - g_prefix_error (error, "%s: ", setting_name); - g_variant_unref (setting_dict); - g_slist_free_full (settings, g_object_unref); - return FALSE; - } - - setting = _nm_setting_new_from_dbus (type, setting_dict, new_settings, error); - g_variant_unref (setting_dict); - - if (!setting) { - g_slist_free_full (settings, g_object_unref); - return FALSE; - } - - settings = g_slist_prepend (settings, setting); - } - - if (g_hash_table_size (priv->settings) > 0) { - g_hash_table_foreach_remove (priv->settings, _setting_release, connection); - changed = TRUE; - } else - changed = (settings != NULL); - - for (s = settings; s; s = s->next) - _nm_connection_add_setting (connection, s->data); - - g_slist_free (settings); - - if (changed) - g_signal_emit (connection, signals[CHANGED], 0); - return TRUE; + return _nm_connection_replace_settings (connection, new_settings, NM_SETTING_PARSE_FLAGS_NONE, error); } /** diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 92de9d1313..60c3c55294 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -100,6 +100,20 @@ NM_SETTING_SECRET_FLAG_NOT_SAVED | \ NM_SETTING_SECRET_FLAG_NOT_REQUIRED) +typedef enum { /*< skip >*/ + NM_SETTING_PARSE_FLAGS_NONE = 0, + NM_SETTING_PARSE_FLAGS_STRICT = 1LL << 0, + NM_SETTING_PARSE_FLAGS_BEST_EFFORT = 1LL << 1, + + _NM_SETTING_PARSE_FLAGS_LAST, + NM_SETTING_PARSE_FLAGS_ALL = ((_NM_SETTING_PARSE_FLAGS_LAST - 1) << 1) - 1, +} NMSettingParseFlags; + +gboolean _nm_connection_replace_settings (NMConnection *connection, + GVariant *new_settings, + NMSettingParseFlags parse_flags, + GError **error); + guint32 _nm_setting_get_setting_priority (NMSetting *setting); gboolean _nm_setting_get_property (NMSetting *setting, const char *name, GValue *value); diff --git a/libnm-core/nm-setting-connection.c b/libnm-core/nm-setting-connection.c index 44237d9801..e1cd913aad 100644 --- a/libnm-core/nm-setting-connection.c +++ b/libnm-core/nm-setting-connection.c @@ -1055,11 +1055,13 @@ find_virtual_interface_name (GVariant *connection_dict) return interface_name; } -static void +static gboolean nm_setting_connection_set_interface_name (NMSetting *setting, GVariant *connection_dict, const char *property, - GVariant *value) + GVariant *value, + NMSettingParseFlags parse_flags, + GError **error) { const char *interface_name; @@ -1074,12 +1076,16 @@ nm_setting_connection_set_interface_name (NMSetting *setting, g_object_set (G_OBJECT (setting), NM_SETTING_CONNECTION_INTERFACE_NAME, interface_name, NULL); + + return TRUE; } -static void +static gboolean nm_setting_connection_no_interface_name (NMSetting *setting, GVariant *connection_dict, - const char *property) + const char *property, + NMSettingParseFlags parse_flags, + GError **error) { const char *virtual_interface_name; @@ -1087,6 +1093,7 @@ nm_setting_connection_no_interface_name (NMSetting *setting, g_object_set (G_OBJECT (setting), NM_SETTING_CONNECTION_INTERFACE_NAME, virtual_interface_name, NULL); + return TRUE; } static gboolean diff --git a/libnm-core/nm-setting-ip-config.c b/libnm-core/nm-setting-ip-config.c index d76179c8e3..a69045b36e 100644 --- a/libnm-core/nm-setting-ip-config.c +++ b/libnm-core/nm-setting-ip-config.c @@ -2464,17 +2464,22 @@ get_property (GObject *object, guint prop_id, } } -static void +static gboolean ip_gateway_set (NMSetting *setting, GVariant *connection_dict, const char *property, - GVariant *value) + GVariant *value, + NMSettingParseFlags parse_flags, + GError **error) { + /* FIXME: properly handle errors */ + /* Don't set from 'gateway' if we're going to use the gateway in 'addresses' */ if (_nm_setting_use_legacy_property (setting, connection_dict, "addresses", "gateway")) - return; + return TRUE; g_object_set (setting, property, g_variant_get_string (value, NULL), NULL); + return TRUE; } static void diff --git a/libnm-core/nm-setting-ip4-config.c b/libnm-core/nm-setting-ip4-config.c index b0308e7162..9b479a083d 100644 --- a/libnm-core/nm-setting-ip4-config.c +++ b/libnm-core/nm-setting-ip4-config.c @@ -313,19 +313,23 @@ ip4_addresses_get (NMSetting *setting, return ret; } -static void +static gboolean ip4_addresses_set (NMSetting *setting, GVariant *connection_dict, const char *property, - GVariant *value) + GVariant *value, + NMSettingParseFlags parse_flags, + GError **error) { GPtrArray *addrs; GVariant *s_ip4; char **labels, *gateway = NULL; int i; + /* FIXME: properly handle errors */ + if (!_nm_setting_use_legacy_property (setting, connection_dict, "addresses", "address-data")) - return; + return TRUE; addrs = nm_utils_ip4_addresses_from_variant (value, &gateway); @@ -344,6 +348,7 @@ ip4_addresses_set (NMSetting *setting, NULL); g_ptr_array_unref (addrs); g_free (gateway); + return TRUE; } static GVariant * @@ -399,21 +404,26 @@ ip4_address_data_get (NMSetting *setting, return ret; } -static void +static gboolean ip4_address_data_set (NMSetting *setting, GVariant *connection_dict, const char *property, - GVariant *value) + GVariant *value, + NMSettingParseFlags parse_flags, + GError **error) { GPtrArray *addrs; + /* FIXME: properly handle errors */ + /* Ignore 'address-data' if we're going to process 'addresses' */ if (_nm_setting_use_legacy_property (setting, connection_dict, "addresses", "address-data")) - return; + return TRUE; addrs = nm_utils_ip_addresses_from_variant (value, AF_INET); g_object_set (setting, NM_SETTING_IP_CONFIG_ADDRESSES, addrs, NULL); g_ptr_array_unref (addrs); + return TRUE; } static GVariant * @@ -430,20 +440,25 @@ ip4_routes_get (NMSetting *setting, return ret; } -static void +static gboolean ip4_routes_set (NMSetting *setting, GVariant *connection_dict, const char *property, - GVariant *value) + GVariant *value, + NMSettingParseFlags parse_flags, + GError **error) { GPtrArray *routes; + /* FIXME: properly handle errors */ + if (!_nm_setting_use_legacy_property (setting, connection_dict, "routes", "route-data")) - return; + return TRUE; routes = nm_utils_ip4_routes_from_variant (value); g_object_set (setting, property, routes, NULL); g_ptr_array_unref (routes); + return TRUE; } static GVariant * @@ -461,21 +476,26 @@ ip4_route_data_get (NMSetting *setting, return ret; } -static void +static gboolean ip4_route_data_set (NMSetting *setting, GVariant *connection_dict, const char *property, - GVariant *value) + GVariant *value, + NMSettingParseFlags parse_flags, + GError **error) { GPtrArray *routes; + /* FIXME: properly handle errors */ + /* Ignore 'route-data' if we're going to process 'routes' */ if (_nm_setting_use_legacy_property (setting, connection_dict, "routes", "route-data")) - return; + return TRUE; routes = nm_utils_ip_routes_from_variant (value, AF_INET); g_object_set (setting, NM_SETTING_IP_CONFIG_ROUTES, routes, NULL); g_ptr_array_unref (routes); + return TRUE; } diff --git a/libnm-core/nm-setting-ip6-config.c b/libnm-core/nm-setting-ip6-config.c index f2d85084f3..bab8c535c7 100644 --- a/libnm-core/nm-setting-ip6-config.c +++ b/libnm-core/nm-setting-ip6-config.c @@ -239,17 +239,21 @@ ip6_addresses_get (NMSetting *setting, return ret; } -static void +static gboolean ip6_addresses_set (NMSetting *setting, GVariant *connection_dict, const char *property, - GVariant *value) + GVariant *value, + NMSettingParseFlags parse_flags, + GError **error) { GPtrArray *addrs; char *gateway = NULL; + /* FIXME: properly handle errors */ + if (!_nm_setting_use_legacy_property (setting, connection_dict, "addresses", "address-data")) - return; + return TRUE; addrs = nm_utils_ip6_addresses_from_variant (value, &gateway); @@ -259,6 +263,7 @@ ip6_addresses_set (NMSetting *setting, NULL); g_ptr_array_unref (addrs); g_free (gateway); + return TRUE; } static GVariant * @@ -276,21 +281,26 @@ ip6_address_data_get (NMSetting *setting, return ret; } -static void +static gboolean ip6_address_data_set (NMSetting *setting, GVariant *connection_dict, const char *property, - GVariant *value) + GVariant *value, + NMSettingParseFlags parse_flags, + GError **error) { GPtrArray *addrs; + /* FIXME: properly handle errors */ + /* Ignore 'address-data' if we're going to process 'addresses' */ if (_nm_setting_use_legacy_property (setting, connection_dict, "addresses", "address-data")) - return; + return TRUE; addrs = nm_utils_ip_addresses_from_variant (value, AF_INET6); g_object_set (setting, NM_SETTING_IP_CONFIG_ADDRESSES, addrs, NULL); g_ptr_array_unref (addrs); + return TRUE; } static GVariant * @@ -307,20 +317,25 @@ ip6_routes_get (NMSetting *setting, return ret; } -static void +static gboolean ip6_routes_set (NMSetting *setting, GVariant *connection_dict, const char *property, - GVariant *value) + GVariant *value, + NMSettingParseFlags parse_flags, + GError **error) { GPtrArray *routes; + /* FIXME: properly handle errors */ + if (!_nm_setting_use_legacy_property (setting, connection_dict, "routes", "route-data")) - return; + return TRUE; routes = nm_utils_ip6_routes_from_variant (value); g_object_set (setting, property, routes, NULL); g_ptr_array_unref (routes); + return TRUE; } static GVariant * @@ -338,21 +353,26 @@ ip6_route_data_get (NMSetting *setting, return ret; } -static void +static gboolean ip6_route_data_set (NMSetting *setting, GVariant *connection_dict, const char *property, - GVariant *value) + GVariant *value, + NMSettingParseFlags parse_flags, + GError **error) { GPtrArray *routes; + /* FIXME: properly handle errors */ + /* Ignore 'route-data' if we're going to process 'routes' */ if (_nm_setting_use_legacy_property (setting, connection_dict, "routes", "route-data")) - return; + return TRUE; routes = nm_utils_ip_routes_from_variant (value, AF_INET6); g_object_set (setting, NM_SETTING_IP_CONFIG_ROUTES, routes, NULL); g_ptr_array_unref (routes); + return TRUE; } static void diff --git a/libnm-core/nm-setting-private.h b/libnm-core/nm-setting-private.h index cb7ca52e3d..6560c4a296 100644 --- a/libnm-core/nm-setting-private.h +++ b/libnm-core/nm-setting-private.h @@ -126,6 +126,7 @@ GVariant *_nm_setting_to_dbus (NMSetting *setting, NMSetting *_nm_setting_new_from_dbus (GType setting_type, GVariant *setting_dict, GVariant *connection_dict, + NMSettingParseFlags parse_flags, GError **error); typedef GVariant * (*NMSettingPropertyGetFunc) (NMSetting *setting, @@ -133,13 +134,17 @@ typedef GVariant * (*NMSettingPropertyGetFunc) (NMSetting *setting, typedef GVariant * (*NMSettingPropertySynthFunc) (NMSetting *setting, NMConnection *connection, const char *property); -typedef void (*NMSettingPropertySetFunc) (NMSetting *setting, +typedef gboolean (*NMSettingPropertySetFunc) (NMSetting *setting, GVariant *connection_dict, const char *property, - GVariant *value); -typedef void (*NMSettingPropertyNotSetFunc) (NMSetting *setting, + GVariant *value, + NMSettingParseFlags parse_flags, + GError **error); +typedef gboolean (*NMSettingPropertyNotSetFunc) (NMSetting *setting, GVariant *connection_dict, - const char *property); + const char *property, + NMSettingParseFlags parse_flags, + GError **error); void _nm_setting_class_add_dbus_only_property (NMSettingClass *setting_class, const char *property_name, diff --git a/libnm-core/nm-setting-vlan.c b/libnm-core/nm-setting-vlan.c index 39587d59f9..827a47af23 100644 --- a/libnm-core/nm-setting-vlan.c +++ b/libnm-core/nm-setting-vlan.c @@ -692,16 +692,19 @@ _override_flags_get (NMSetting *setting, const char *property) return g_variant_new_uint32 (nm_setting_vlan_get_flags ((NMSettingVlan *) setting)); } -static void +static gboolean _override_flags_not_set (NMSetting *setting, - GVariant *connection_dict, - const char *property) + GVariant *connection_dict, + const char *property, + NMSettingParseFlags parse_flags, + GError **error) { /* we changed the default value for FLAGS. When an older client * doesn't serialize the property, we assume it is the old default. */ g_object_set (G_OBJECT (setting), NM_SETTING_VLAN_FLAGS, (NMVlanFlags) 0, NULL); + return TRUE; } static GSList * diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index fec645b263..0c41bd84c3 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -774,6 +774,7 @@ _nm_setting_to_dbus (NMSetting *setting, NMConnection *connection, NMConnectionS * mapping property names to values * @connection_dict: the #GVariant containing an %NM_VARIANT_TYPE_CONNECTION * dictionary mapping setting names to dictionaries. + * @parse_flags: flags to determine behavior during parsing. * @error: location to store error, or %NULL * * Creates a new #NMSetting object and populates that object with the properties @@ -790,16 +791,20 @@ NMSetting * _nm_setting_new_from_dbus (GType setting_type, GVariant *setting_dict, GVariant *connection_dict, + NMSettingParseFlags parse_flags, GError **error) { - NMSetting *setting; + gs_unref_object NMSetting *setting = NULL; + gs_unref_hashtable GHashTable *keys = NULL; const NMSettingProperty *properties; - guint n_properties; - guint i; + guint i, n_properties; g_return_val_if_fail (G_TYPE_IS_INSTANTIATABLE (setting_type), NULL); g_return_val_if_fail (g_variant_is_of_type (setting_dict, NM_VARIANT_TYPE_SETTING), NULL); + nm_assert (!NM_FLAGS_ANY (parse_flags, ~NM_SETTING_PARSE_FLAGS_ALL)); + nm_assert (!NM_FLAGS_ALL (parse_flags, NM_SETTING_PARSE_FLAGS_STRICT | NM_SETTING_PARSE_FLAGS_BEST_EFFORT)); + /* connection_dict is not technically optional, but some tests in test-general * don't bother with it in cases where they know it's not needed. */ @@ -813,19 +818,49 @@ _nm_setting_new_from_dbus (GType setting_type, */ setting = (NMSetting *) g_object_new (setting_type, NULL); + if (NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_STRICT)) { + GVariantIter iter; + GVariant *entry, *entry_key; + char *key; + + keys = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL); + + g_variant_iter_init (&iter, setting_dict); + while ((entry = g_variant_iter_next_value (&iter))) { + entry_key = g_variant_get_child_value (entry, 0); + key = g_strdup (g_variant_get_string (entry_key, NULL)); + g_variant_unref (entry_key); + g_variant_unref (entry); + + if (!nm_g_hash_table_add (keys, key)) { + g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_SETTING, + _("duplicate property")); + g_prefix_error (error, "%s.%s: ", nm_setting_get_name (setting), key); + return NULL; + } + } + } + properties = nm_setting_class_get_properties (NM_SETTING_GET_CLASS (setting), &n_properties); for (i = 0; i < n_properties; i++) { const NMSettingProperty *property = &properties[i]; - GVariant *value; + gs_unref_variant GVariant *value = NULL; + gs_free_error GError *local = NULL; if (property->param_spec && !(property->param_spec->flags & G_PARAM_WRITABLE)) continue; value = g_variant_lookup_value (setting_dict, property->name, NULL); + if (value && keys) + g_hash_table_remove (keys, property->name); + if (value && property->set_func) { + if (!g_variant_type_equal (g_variant_get_type (value), property->dbus_type)) { - property_type_error: + /* for backward behavior, fail unless best-effort is chosen. */ + if (NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_BEST_EFFORT)) + continue; g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, _("can't set property of type '%s' from value of type '%s'"), property->dbus_type ? @@ -834,36 +869,83 @@ _nm_setting_new_from_dbus (GType setting_type, g_type_name (property->param_spec->value_type) : "(unknown)", g_variant_get_type_string (value)); g_prefix_error (error, "%s.%s: ", nm_setting_get_name (setting), property->name); - - g_variant_unref (value); - g_object_unref (setting); return NULL; } - property->set_func (setting, - connection_dict, - property->name, - value); + if (!property->set_func (setting, + connection_dict, + property->name, + value, + parse_flags, + &local)) { + if (!NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_STRICT)) + continue; + g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("failed to set property: %s"), + local->message); + g_prefix_error (error, "%s.%s: ", nm_setting_get_name (setting), property->name); + return NULL; + } } else if (!value && property->not_set_func) { - property->not_set_func (setting, - connection_dict, - property->name); + if (!property->not_set_func (setting, + connection_dict, + property->name, + parse_flags, + &local)) { + if (!NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_STRICT)) + continue; + g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("failed to set property: %s"), + local->message); + g_prefix_error (error, "%s.%s: ", nm_setting_get_name (setting), property->name); + return NULL; + } } else if (value && property->param_spec) { - GValue object_value = { 0, }; + nm_auto_unset_gvalue GValue object_value = G_VALUE_INIT; g_value_init (&object_value, property->param_spec->value_type); - if (!set_property_from_dbus (property, value, &object_value)) - goto property_type_error; + if (!set_property_from_dbus (property, value, &object_value)) { + /* for backward behavior, fail unless best-effort is chosen. */ + if (NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_BEST_EFFORT)) + continue; + g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("can't set property of type '%s' from value of type '%s'"), + property->dbus_type ? + g_variant_type_peek_string (property->dbus_type) : + property->param_spec ? + g_type_name (property->param_spec->value_type) : "(unknown)", + g_variant_get_type_string (value)); + g_prefix_error (error, "%s.%s: ", nm_setting_get_name (setting), property->name); + return NULL; + } - g_object_set_property (G_OBJECT (setting), property->param_spec->name, &object_value); - g_value_unset (&object_value); + if (!nm_g_object_set_property (G_OBJECT (setting), property->param_spec->name, &object_value, &local)) { + if (!NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_STRICT)) + continue; + g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("can not set property: %s"), + local->message); + g_prefix_error (error, "%s.%s: ", nm_setting_get_name (setting), property->name); + return NULL; + } } - - if (value) - g_variant_unref (value); } - return setting; + if ( NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_STRICT) + && g_hash_table_size (keys) > 0) { + GHashTableIter iter; + const char *key; + + g_hash_table_iter_init (&iter, keys); + if (g_hash_table_iter_next (&iter, (gpointer *) &key, NULL)) { + g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("unknown property")); + g_prefix_error (error, "%s.%s: ", nm_setting_get_name (setting), key); + return NULL; + } + } + + return nm_unauto (&setting); } /** diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index 82af4140f1..6541834284 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -1025,7 +1025,7 @@ test_setting_new_from_dbus (void) dict = _nm_setting_to_dbus (NM_SETTING (s_wsec), NULL, NM_CONNECTION_SERIALIZE_ALL); g_object_unref (s_wsec); - s_wsec = (NMSettingWirelessSecurity *) _nm_setting_new_from_dbus (NM_TYPE_SETTING_WIRELESS_SECURITY, dict, NULL, NULL); + s_wsec = (NMSettingWirelessSecurity *) _nm_setting_new_from_dbus (NM_TYPE_SETTING_WIRELESS_SECURITY, dict, NULL, NM_SETTING_PARSE_FLAGS_NONE, NULL); g_variant_unref (dict); g_assert (s_wsec); @@ -1054,7 +1054,7 @@ test_setting_new_from_dbus_transform (void) dbus_mac_address, ETH_ALEN, 1)); dict = g_variant_builder_end (&builder); - s_wired = _nm_setting_new_from_dbus (NM_TYPE_SETTING_WIRED, dict, NULL, &error); + s_wired = _nm_setting_new_from_dbus (NM_TYPE_SETTING_WIRED, dict, NULL, NM_SETTING_PARSE_FLAGS_NONE, &error); g_assert_no_error (error); g_assert_cmpstr (nm_setting_wired_get_mac_address (NM_SETTING_WIRED (s_wired)), ==, test_mac_address); @@ -1080,7 +1080,7 @@ test_setting_new_from_dbus_enum (void) g_variant_new_int32 (NM_SETTING_IP6_CONFIG_PRIVACY_PREFER_TEMP_ADDR)); dict = g_variant_builder_end (&builder); - s_ip6 = (NMSettingIP6Config *) _nm_setting_new_from_dbus (NM_TYPE_SETTING_IP6_CONFIG, dict, NULL, &error); + s_ip6 = (NMSettingIP6Config *) _nm_setting_new_from_dbus (NM_TYPE_SETTING_IP6_CONFIG, dict, NULL, NM_SETTING_PARSE_FLAGS_NONE, &error); g_assert_no_error (error); g_assert_cmpint (nm_setting_ip6_config_get_ip6_privacy (s_ip6), ==, NM_SETTING_IP6_CONFIG_PRIVACY_PREFER_TEMP_ADDR); @@ -1099,7 +1099,7 @@ test_setting_new_from_dbus_enum (void) NM_SETTING_SECRET_FLAG_NOT_SAVED)); dict = g_variant_builder_end (&builder); - s_wsec = (NMSettingWirelessSecurity *) _nm_setting_new_from_dbus (NM_TYPE_SETTING_WIRELESS_SECURITY, dict, NULL, &error); + s_wsec = (NMSettingWirelessSecurity *) _nm_setting_new_from_dbus (NM_TYPE_SETTING_WIRELESS_SECURITY, dict, NULL, NM_SETTING_PARSE_FLAGS_NONE, &error); g_assert_no_error (error); g_assert_cmpint (nm_setting_wireless_security_get_wep_key_type (s_wsec), ==, NM_WEP_KEY_TYPE_KEY); @@ -1116,7 +1116,7 @@ test_setting_new_from_dbus_enum (void) g_variant_new_byte ('E')); dict = g_variant_builder_end (&builder); - s_serial = (NMSettingSerial *) _nm_setting_new_from_dbus (NM_TYPE_SETTING_SERIAL, dict, NULL, &error); + s_serial = (NMSettingSerial *) _nm_setting_new_from_dbus (NM_TYPE_SETTING_SERIAL, dict, NULL, NM_SETTING_PARSE_FLAGS_NONE, &error); g_assert_no_error (error); g_assert_cmpint (nm_setting_serial_get_parity (s_serial), ==, NM_SETTING_SERIAL_PARITY_EVEN); From 3d8776108c6cf2a2831d58e45e0f1b19a5e0634f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 17 Mar 2016 10:34:44 +0100 Subject: [PATCH 12/28] libnm-core: add _nm_simple_connection_new_from_dbus() function Contary to nm_simple_connection_new_from_dbus(), this internal function allows to specify parse-flags. --- libnm-core/nm-connection.c | 19 +++++++++++++-- libnm-core/nm-core-internal.h | 5 ++++ libnm-core/nm-simple-connection.c | 34 ++++++++++++++++++++++----- src/settings/nm-settings-connection.c | 4 +++- src/settings/nm-settings.c | 4 +++- 5 files changed, 56 insertions(+), 10 deletions(-) diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c index f0b2387bac..c03e62656f 100644 --- a/libnm-core/nm-connection.c +++ b/libnm-core/nm-connection.c @@ -245,6 +245,9 @@ validate_permissions_type (GVariant *variant, GError **error) * * Returns: %TRUE if connection was updated, %FALSE if @new_settings could not * be deserialized (in which case @connection will be unchanged). + * Only exception is the NM_SETTING_PARSE_FLAGS_NORMALIZE flag: if normalization + * fails, the input @connection is already modified and the original settings + * are lost. **/ gboolean _nm_connection_replace_settings (NMConnection *connection, @@ -257,7 +260,7 @@ _nm_connection_replace_settings (NMConnection *connection, const char *setting_name; GVariant *setting_dict; GSList *settings = NULL, *s; - gboolean changed; + gboolean changed, success; g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE); g_return_val_if_fail (g_variant_is_of_type (new_settings, NM_VARIANT_TYPE_CONNECTION), FALSE); @@ -339,9 +342,21 @@ _nm_connection_replace_settings (NMConnection *connection, g_slist_free (settings); + /* If verification/normalization fails, the original connection + * is already lost. From an API point of view, it would be nicer + * not to touch the input argument if we fail at the end. + * However, that would require creating a temporary connection + * to validate it first. As none of the caller cares about the + * state of the @connection when normalization fails, just do it + * this way. */ + if (NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_NORMALIZE)) + success = nm_connection_normalize (connection, NULL, NULL, error); + else + success = TRUE; + if (changed) g_signal_emit (connection, signals[CHANGED], 0); - return TRUE; + return success; } /** diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 60c3c55294..9512ee5619 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -104,6 +104,7 @@ typedef enum { /*< skip >*/ NM_SETTING_PARSE_FLAGS_NONE = 0, NM_SETTING_PARSE_FLAGS_STRICT = 1LL << 0, NM_SETTING_PARSE_FLAGS_BEST_EFFORT = 1LL << 1, + NM_SETTING_PARSE_FLAGS_NORMALIZE = 1LL << 2, _NM_SETTING_PARSE_FLAGS_LAST, NM_SETTING_PARSE_FLAGS_ALL = ((_NM_SETTING_PARSE_FLAGS_LAST - 1) << 1) - 1, @@ -114,6 +115,10 @@ gboolean _nm_connection_replace_settings (NMConnection *connection, NMSettingParseFlags parse_flags, GError **error); +NMConnection *_nm_simple_connection_new_from_dbus (GVariant *dict, + NMSettingParseFlags parse_flags, + GError **error); + guint32 _nm_setting_get_setting_priority (NMSetting *setting); gboolean _nm_setting_get_property (NMSetting *setting, const char *name, GValue *value); diff --git a/libnm-core/nm-simple-connection.c b/libnm-core/nm-simple-connection.c index 1036c8ecce..11700666f5 100644 --- a/libnm-core/nm-simple-connection.c +++ b/libnm-core/nm-simple-connection.c @@ -51,7 +51,7 @@ nm_simple_connection_new (void) } /** - * nm_simple_connection_new_from_dbus: + * _nm_simple_connection_new_from_dbus: * @dict: a #GVariant of type %NM_VARIANT_TYPE_CONNECTION describing the connection * @error: on unsuccessful return, an error * @@ -60,24 +60,46 @@ nm_simple_connection_new (void) * hash table. * * Returns: (transfer full): the new #NMSimpleConnection object, populated with - * settings created from the values in the hash table, or %NULL if the - * connection failed to validate + * settings created from the values in the hash table, or %NULL if there was + * an error. **/ NMConnection * -nm_simple_connection_new_from_dbus (GVariant *dict, GError **error) +_nm_simple_connection_new_from_dbus (GVariant *dict, NMSettingParseFlags parse_flags, GError **error) { NMConnection *connection; g_return_val_if_fail (dict != NULL, NULL); g_return_val_if_fail (g_variant_is_of_type (dict, NM_VARIANT_TYPE_CONNECTION), NULL); + g_return_val_if_fail (!NM_FLAGS_ANY (parse_flags, ~NM_SETTING_PARSE_FLAGS_ALL), NULL); + g_return_val_if_fail (!NM_FLAGS_ALL (parse_flags, NM_SETTING_PARSE_FLAGS_STRICT | NM_SETTING_PARSE_FLAGS_BEST_EFFORT), NULL); connection = nm_simple_connection_new (); - if ( !nm_connection_replace_settings (connection, dict, error) - || !nm_connection_normalize (connection, NULL, NULL, error)) + if (!_nm_connection_replace_settings (connection, dict, parse_flags, error)) g_clear_object (&connection); return connection; } +/** + * nm_simple_connection_new_from_dbus: + * @dict: a #GVariant of type %NM_VARIANT_TYPE_CONNECTION describing the connection + * @error: on unsuccessful return, an error + * + * Creates a new #NMSimpleConnection from a hash table describing the + * connection and normalize the connection. See nm_connection_to_dbus() for a + * description of the expected hash table. + * + * Returns: (transfer full): the new #NMSimpleConnection object, populated with + * settings created from the values in the hash table, or %NULL if the + * connection failed to normalize. + **/ +NMConnection * +nm_simple_connection_new_from_dbus (GVariant *dict, GError **error) +{ + return _nm_simple_connection_new_from_dbus (dict, + NM_SETTING_PARSE_FLAGS_NORMALIZE, + error); +} + /** * nm_simple_connection_new_clone: * @connection: the #NMConnection to clone diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 10aa77ab41..bce3dff104 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -1735,7 +1735,9 @@ settings_connection_update_helper (NMSettingsConnection *self, /* Check if the settings are valid first */ if (new_settings) { - tmp = nm_simple_connection_new_from_dbus (new_settings, &error); + tmp = _nm_simple_connection_new_from_dbus (new_settings, + NM_SETTING_PARSE_FLAGS_NORMALIZE, + &error); if (!tmp) goto error; } diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index f090185480..fc2daa063c 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1407,7 +1407,9 @@ impl_settings_add_connection_helper (NMSettings *self, NMConnection *connection; GError *error = NULL; - connection = nm_simple_connection_new_from_dbus (settings, &error); + connection = _nm_simple_connection_new_from_dbus (settings, + NM_SETTING_PARSE_FLAGS_NORMALIZE, + &error); if (connection) { if (!nm_connection_verify_secrets (connection, &error)) From df405942ded81e756a5ef9c75a0b118996b33bc5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2016 16:04:54 +0100 Subject: [PATCH 13/28] libnm-core/tests: refactor call to nm_simple_connection_new_from_dbus() No actual change, let's just not directly call nm_simple_connection_new_from_dbus(). Instead, add a wrapper to define in once place the flags we use for loading the connection. --- libnm-core/tests/test-general.c | 48 ++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index 6541834284..dcbcb88021 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -74,6 +74,12 @@ G_STATIC_ASSERT (sizeof (bool) <= sizeof (int)); /*****************************************************************************/ +static NMConnection * +_connection_new_from_dbus (GVariant *dict, GError **error) +{ + return _nm_simple_connection_new_from_dbus (dict, NM_SETTING_PARSE_FLAGS_NORMALIZE, error); +} + static void vpn_check_func (const char *key, const char *value, gpointer user_data) { @@ -450,7 +456,7 @@ test_setting_ip4_config_labels (void) NMTST_VARIANT_DROP_PROPERTY (NM_SETTING_IP4_CONFIG_SETTING_NAME, "address-data"); ); - conn = nm_simple_connection_new_from_dbus (dict, &error); + conn = _connection_new_from_dbus (dict, &error); g_assert_no_error (error); g_variant_unref (dict); @@ -473,7 +479,7 @@ test_setting_ip4_config_labels (void) NMTST_VARIANT_DROP_PROPERTY (NM_SETTING_IP4_CONFIG_SETTING_NAME, "address-labels"); ); - conn = nm_simple_connection_new_from_dbus (dict2, &error); + conn = _connection_new_from_dbus (dict2, &error); g_assert_no_error (error); g_variant_unref (dict2); @@ -603,7 +609,7 @@ test_setting_ip4_config_address_data (void) g_object_unref (conn); /* When we reserialize that dictionary as a client, 'address-data' will be preferred. */ - conn = nm_simple_connection_new_from_dbus (dict, &error); + conn = _connection_new_from_dbus (dict, &error); g_assert_no_error (error); s_ip4 = nm_connection_get_setting_ip4_config (conn); @@ -619,7 +625,7 @@ test_setting_ip4_config_address_data (void) /* But on the server side, 'addresses' will have precedence. */ _nm_utils_is_manager_process = TRUE; - conn = nm_simple_connection_new_from_dbus (dict, &error); + conn = _connection_new_from_dbus (dict, &error); _nm_utils_is_manager_process = FALSE; g_assert_no_error (error); g_variant_unref (dict); @@ -1188,7 +1194,7 @@ test_setting_new_from_dbus_bad (void) g_object_unref (conn); /* sanity-check */ - conn = nm_simple_connection_new_from_dbus (orig_dict, &error); + conn = _connection_new_from_dbus (orig_dict, &error); g_assert_no_error (error); g_assert (conn); g_object_unref (conn); @@ -1201,7 +1207,7 @@ test_setting_new_from_dbus_bad (void) NM_SETTING_WIRELESS_RATE, "i", 10); ); - conn = nm_simple_connection_new_from_dbus (dict, &error); + conn = _connection_new_from_dbus (dict, &error); g_assert (conn); g_assert_no_error (error); setting = nm_connection_get_setting (conn, NM_TYPE_SETTING_WIRELESS); @@ -1216,7 +1222,7 @@ test_setting_new_from_dbus_bad (void) NM_SETTING_IP6_CONFIG_IP6_PRIVACY, "i", NM_SETTING_IP6_CONFIG_PRIVACY_PREFER_TEMP_ADDR); ); - conn = nm_simple_connection_new_from_dbus (dict, &error); + conn = _connection_new_from_dbus (dict, &error); g_assert (conn); g_assert_no_error (error); setting = nm_connection_get_setting (conn, NM_TYPE_SETTING_IP6_CONFIG); @@ -1233,7 +1239,7 @@ test_setting_new_from_dbus_bad (void) NM_SETTING_WIRELESS_RATE, "s", "ten"); ); - conn = nm_simple_connection_new_from_dbus (dict, &error); + conn = _connection_new_from_dbus (dict, &error); g_assert_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY); g_assert (g_str_has_prefix (error->message, "802-11-wireless.rate:")); g_clear_error (&error); @@ -1245,7 +1251,7 @@ test_setting_new_from_dbus_bad (void) NM_SETTING_WIRELESS_MODE, "b", FALSE); ); - conn = nm_simple_connection_new_from_dbus (dict, &error); + conn = _connection_new_from_dbus (dict, &error); g_assert_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY); g_assert (g_str_has_prefix (error->message, "802-11-wireless.mode:")); g_clear_error (&error); @@ -1257,7 +1263,7 @@ test_setting_new_from_dbus_bad (void) NM_SETTING_WIRELESS_SSID, "s", "fred"); ); - conn = nm_simple_connection_new_from_dbus (dict, &error); + conn = _connection_new_from_dbus (dict, &error); g_assert_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY); g_assert (g_str_has_prefix (error->message, "802-11-wireless.ssid:")); g_clear_error (&error); @@ -1269,7 +1275,7 @@ test_setting_new_from_dbus_bad (void) NM_SETTING_WIRELESS_BSSID, "i", 42); ); - conn = nm_simple_connection_new_from_dbus (dict, &error); + conn = _connection_new_from_dbus (dict, &error); g_assert_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY); g_assert (g_str_has_prefix (error->message, "802-11-wireless.bssid:")); g_clear_error (&error); @@ -1281,7 +1287,7 @@ test_setting_new_from_dbus_bad (void) NM_SETTING_IP6_CONFIG_IP6_PRIVACY, "s", "private"); ); - conn = nm_simple_connection_new_from_dbus (dict, &error); + conn = _connection_new_from_dbus (dict, &error); g_assert_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY); g_assert (g_str_has_prefix (error->message, "ipv6.ip6-privacy:")); g_clear_error (&error); @@ -1293,7 +1299,7 @@ test_setting_new_from_dbus_bad (void) NM_SETTING_IP_CONFIG_ADDRESSES, "s", "1234::5678"); ); - conn = nm_simple_connection_new_from_dbus (dict, &error); + conn = _connection_new_from_dbus (dict, &error); g_assert_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY); g_assert (g_str_has_prefix (error->message, "ipv6.addresses:")); g_clear_error (&error); @@ -1561,7 +1567,7 @@ test_connection_new_from_dbus (void) g_assert (new_settings); /* Replace settings and test */ - connection = nm_simple_connection_new_from_dbus (new_settings, &error); + connection = _connection_new_from_dbus (new_settings, &error); g_assert_no_error (error); g_assert (connection); @@ -3241,7 +3247,7 @@ test_connection_normalize_virtual_iface_name (void) ":::this-is-not-a-valid-interface-name:::"); ); - con = nm_simple_connection_new_from_dbus (connection_dict, &error); + con = _connection_new_from_dbus (connection_dict, &error); g_assert_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY); g_clear_error (&error); @@ -3253,7 +3259,7 @@ test_connection_normalize_virtual_iface_name (void) IFACE_VIRT); ); - con = nm_simple_connection_new_from_dbus (connection_dict, &error); + con = _connection_new_from_dbus (connection_dict, &error); g_assert_no_error (error); g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_NAME); @@ -3269,7 +3275,7 @@ test_connection_normalize_virtual_iface_name (void) NM_SETTING_CONNECTION_INTERFACE_NAME); ); - con = nm_simple_connection_new_from_dbus (connection_dict, &error); + con = _connection_new_from_dbus (connection_dict, &error); g_assert_no_error (error); g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_VIRT); @@ -3783,7 +3789,7 @@ test_setting_ip4_gateway (void) "address-data"); ); - conn = nm_simple_connection_new_from_dbus (conn_dict, &error); + conn = _connection_new_from_dbus (conn_dict, &error); g_assert_no_error (error); s_ip4 = (NMSettingIPConfig *) nm_connection_get_setting_ip4_config (conn); @@ -3805,7 +3811,7 @@ test_setting_ip4_gateway (void) "addresses", "aau", &addrs_builder); ); - conn = nm_simple_connection_new_from_dbus (conn_dict, &error); + conn = _connection_new_from_dbus (conn_dict, &error); g_assert_no_error (error); g_variant_unref (conn_dict); @@ -3890,7 +3896,7 @@ test_setting_ip6_gateway (void) "address-data"); ); - conn = nm_simple_connection_new_from_dbus (conn_dict, &error); + conn = _connection_new_from_dbus (conn_dict, &error); g_assert_no_error (error); s_ip6 = (NMSettingIPConfig *) nm_connection_get_setting_ip6_config (conn); @@ -3918,7 +3924,7 @@ test_setting_ip6_gateway (void) "addresses", "a(ayuay)", &addrs_builder); ); - conn = nm_simple_connection_new_from_dbus (conn_dict, &error); + conn = _connection_new_from_dbus (conn_dict, &error); g_assert_no_error (error); g_variant_unref (conn_dict); From a37c1d1e17e3161b61f51816018ad10af1fa569b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 17 Mar 2016 10:37:42 +0100 Subject: [PATCH 14/28] libnm: don't normalize connection for nm_device_get_applied_connection() Normalizing means that we fail on invalid connections. Which can happen when the server is newer than the libnm version. We just want to return whatever we can. The caller should make sense of this. This makes libnm more accepting and thus is not going to break existing applications. Also, nm_device_get_applied_connection() is new API since nm-1-1. --- libnm/nm-device.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libnm/nm-device.c b/libnm/nm-device.c index 81077bee4d..5679fd5f3d 100644 --- a/libnm/nm-device.c +++ b/libnm/nm-device.c @@ -2355,7 +2355,7 @@ nm_device_get_applied_connection (NMDevice *device, return NULL; } - connection = nm_simple_connection_new_from_dbus (dict, error); + connection = _nm_simple_connection_new_from_dbus (dict, NM_SETTING_PARSE_FLAGS_BEST_EFFORT, error); if (!connection) return NULL; @@ -2397,7 +2397,7 @@ device_get_applied_connection_cb (GObject *proxy, goto out; } - connection = nm_simple_connection_new_from_dbus (dict, &error); + connection = _nm_simple_connection_new_from_dbus (dict, NM_SETTING_PARSE_FLAGS_BEST_EFFORT, &error); if (!connection) { g_simple_async_result_take_error (simple, error); goto out; From 9a31bbcbc398568a065e0f3d48c0dfce7eddcef7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2016 15:32:43 +0100 Subject: [PATCH 15/28] libnm: accept invalid connections in NMSecretAgentOld When we receive a connection from NetworkManager it is not guaranteed that the connection verifies. For example, if the current libnm version is older then the NetworkManager version. Be more accepting and don't do any verification of the connection. This is a change in behavior in that we accept also invalid connections and pass them down to the sub-classes. --- libnm/nm-secret-agent-old.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libnm/nm-secret-agent-old.c b/libnm/nm-secret-agent-old.c index 8edb224cb1..60b7a624e2 100644 --- a/libnm/nm-secret-agent-old.c +++ b/libnm/nm-secret-agent-old.c @@ -27,6 +27,7 @@ #include "nm-enum-types.h" #include "nm-dbus-helpers.h" #include "nm-simple-connection.h" +#include "nm-core-internal.h" #include "nmdbus-secret-agent.h" #include "nmdbus-agent-manager.h" @@ -273,7 +274,7 @@ verify_request (NMSecretAgentOld *self, /* Make sure the given connection is valid */ g_assert (out_connection); - connection = nm_simple_connection_new_from_dbus (connection_dict, &local); + connection = _nm_simple_connection_new_from_dbus (connection_dict, NM_SETTING_PARSE_FLAGS_BEST_EFFORT, &local); if (connection) { nm_connection_set_path (connection, connection_path); *out_connection = connection; From 559ab7bd7cd886b97d23ae43b3870f71cb147d62 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2016 15:39:49 +0100 Subject: [PATCH 16/28] libnm: accept invalid connections in NMVpnServicePlugin When we receive a connection from NetworkManager it is not guaranteed that the connection verifies. For example, if the current libnm version is older then the NetworkManager version. Be more accepting and don't do any verification of the connection. For NMVpnPluginOld this change is uncritical, because there are probably no users of this API anyway. NMVpnServicePlugin is new API since nm-1-1. However, this API is already strongly used by all the plugins we ported over. So this change is affecting them. This should only matter if libnm's and NetworkManager's version differ, because NetworkManager just doesn't send out an invalid connection. It actually only matters if NetworkManager is a newer version and sends an invalid connection to the client. That is anyway badly tested and probably this changes rather improves compatibility than breaking existing users. --- libnm/nm-vpn-plugin-old.c | 6 +++--- libnm/nm-vpn-service-plugin.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/libnm/nm-vpn-plugin-old.c b/libnm/nm-vpn-plugin-old.c index 74861edc46..634e61a232 100644 --- a/libnm/nm-vpn-plugin-old.c +++ b/libnm/nm-vpn-plugin-old.c @@ -465,7 +465,7 @@ _connect_generic (NMVpnPluginOld *plugin, return; } - connection = nm_simple_connection_new_from_dbus (properties, &error); + connection = _nm_simple_connection_new_from_dbus (properties, NM_SETTING_PARSE_FLAGS_BEST_EFFORT, &error); if (!connection) { g_dbus_method_invocation_return_error (context, NM_VPN_PLUGIN_ERROR, @@ -547,7 +547,7 @@ impl_vpn_plugin_old_need_secrets (NMVpnPluginOld *plugin, gboolean needed; GError *error = NULL; - connection = nm_simple_connection_new_from_dbus (properties, &error); + connection = _nm_simple_connection_new_from_dbus (properties, NM_SETTING_PARSE_FLAGS_BEST_EFFORT, &error); if (!connection) { g_dbus_method_invocation_return_error (context, NM_VPN_PLUGIN_ERROR, @@ -606,7 +606,7 @@ impl_vpn_plugin_old_new_secrets (NMVpnPluginOld *plugin, return; } - connection = nm_simple_connection_new_from_dbus (properties, &error); + connection = _nm_simple_connection_new_from_dbus (properties, NM_SETTING_PARSE_FLAGS_BEST_EFFORT, &error); if (!connection) { g_dbus_method_invocation_return_error (context, NM_VPN_PLUGIN_ERROR, diff --git a/libnm/nm-vpn-service-plugin.c b/libnm/nm-vpn-service-plugin.c index 876d479817..82e29ee8a3 100644 --- a/libnm/nm-vpn-service-plugin.c +++ b/libnm/nm-vpn-service-plugin.c @@ -481,7 +481,7 @@ _connect_generic (NMVpnServicePlugin *plugin, return; } - connection = nm_simple_connection_new_from_dbus (properties, &error); + connection = _nm_simple_connection_new_from_dbus (properties, NM_SETTING_PARSE_FLAGS_BEST_EFFORT, &error); if (!connection) { g_dbus_method_invocation_return_error (context, NM_VPN_PLUGIN_ERROR, @@ -567,7 +567,7 @@ impl_vpn_service_plugin_need_secrets (NMVpnServicePlugin *plugin, gboolean needed; GError *error = NULL; - connection = nm_simple_connection_new_from_dbus (properties, &error); + connection = _nm_simple_connection_new_from_dbus (properties, NM_SETTING_PARSE_FLAGS_BEST_EFFORT, &error); if (!connection) { g_dbus_method_invocation_return_error (context, NM_VPN_PLUGIN_ERROR, @@ -626,7 +626,7 @@ impl_vpn_service_plugin_new_secrets (NMVpnServicePlugin *plugin, return; } - connection = nm_simple_connection_new_from_dbus (properties, &error); + connection = _nm_simple_connection_new_from_dbus (properties, NM_SETTING_PARSE_FLAGS_BEST_EFFORT, &error); if (!connection) { g_dbus_method_invocation_return_error (context, NM_VPN_PLUGIN_ERROR, From 4aa7e09d1fca28c31589ab7e9dd1c94bd7b63dcd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2016 16:18:25 +0100 Subject: [PATCH 17/28] libnm: be more accepting for invalid connections from NetworkManager Relax our error checking which will allow us to try harder to make the best out of whatever NetworkManager sends us. Also, drop the g_warning(). First, now we really don't expect this function to fail. And even in that case, raising a g_warning() from the library is not very friendly to the user of libnm. --- libnm/nm-remote-connection.c | 11 +++++------ libnm/tests/test-nm-client.c | 4 ---- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/libnm/nm-remote-connection.c b/libnm/nm-remote-connection.c index c4a0b6aeb0..eb2874a049 100644 --- a/libnm/nm-remote-connection.c +++ b/libnm/nm-remote-connection.c @@ -26,6 +26,7 @@ #include "nm-dbus-interface.h" #include "nm-utils.h" #include "nm-setting-connection.h" +#include "nm-core-internal.h" #include "nm-remote-connection.h" #include "nm-remote-connection-private.h" @@ -563,13 +564,11 @@ replace_settings (NMRemoteConnection *self, GVariant *new_settings) { GError *error = NULL; - if (!nm_connection_replace_settings (NM_CONNECTION (self), new_settings, &error)) { - g_warning ("%s: error updating connection %s settings: %s", - __func__, - nm_connection_get_path (NM_CONNECTION (self)), - error->message); + if (!_nm_connection_replace_settings ((NMConnection *) self, + new_settings, + NM_SETTING_PARSE_FLAGS_BEST_EFFORT, + &error)) g_clear_error (&error); - } } static void diff --git a/libnm/tests/test-nm-client.c b/libnm/tests/test-nm-client.c index c6a4a05cab..3a7c4d5dde 100644 --- a/libnm/tests/test-nm-client.c +++ b/libnm/tests/test-nm-client.c @@ -1309,9 +1309,7 @@ test_connection_invalid (void) FALSE, &path2); - g_test_expect_message ("libnm", G_LOG_LEVEL_WARNING, "*replace_settings: error updating connection /org/freedesktop/NetworkManager/Settings/Connection/3 settings: *"); client = nm_client_new (NULL, &error); - g_test_assert_expected_messages (); g_assert_no_error (error); connections = nm_client_get_connections (client); @@ -1385,9 +1383,7 @@ test_connection_invalid (void) variant, FALSE); - g_test_expect_message ("libnm", G_LOG_LEVEL_WARNING, "*replace_settings: error updating connection /org/freedesktop/NetworkManager/Settings/Connection/3 settings: *"); nmtst_main_loop_run (loop, 100); - g_test_assert_expected_messages (); connections = nm_client_get_connections (client); g_assert (connections); From d4c201272eac243b60d35ee843fa00f323c4d955 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2016 15:50:20 +0100 Subject: [PATCH 18/28] core: be strict about connection argument for Reapply() D-Bus method There is no excuse for clients to send connections to NetworkManager that have invalid/unknown fields. Just reject them. As Reapply() is new API in nm-1-1, there is no problem with backward compatibility. --- src/devices/nm-device.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 4f92ec1f24..33d33fa972 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7369,7 +7369,10 @@ impl_device_reapply (NMDevice *self, if (settings && g_variant_n_children (settings)) { /* New settings specified inline. */ - connection = nm_simple_connection_new_from_dbus (settings, &error); + connection = _nm_simple_connection_new_from_dbus (settings, + NM_SETTING_PARSE_FLAGS_STRICT + | NM_SETTING_PARSE_FLAGS_NORMALIZE, + &error); if (!connection) { g_prefix_error (&error, "The settings specified are invalid: "); nm_audit_log_device_op (NM_AUDIT_OP_DEVICE_REAPPLY, self, FALSE, context, error->message); From 0c5b98b46459a0d4ada4b2c2f9d369a8d3333f3c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2016 16:12:12 +0100 Subject: [PATCH 19/28] core: be strict when parsing connection in AddAndActivateConnection AddAndActivateConnection is allowed to provide an incomplete connection that will be completed by NetworkManager. That is, a connection that does not verify. But we still want to catch invalid properties or unknown setting types. Thus, we want to reject invalid partial connections. This possibly rejects invalid requests from clients that were accepted before. Thus this change has the potential to break misbehaving clients. --- src/nm-manager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 06065536f8..ba52d52f77 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -3599,7 +3599,7 @@ impl_manager_add_and_activate_connection (NMManager *self, */ connection = nm_simple_connection_new (); if (settings && g_variant_n_children (settings)) - nm_connection_replace_settings (connection, settings, NULL); + _nm_connection_replace_settings (connection, settings, NM_SETTING_PARSE_FLAGS_STRICT, NULL); subject = validate_activation_request (self, context, From 7991298c51fba372b6f45d6ec58bef584942c99b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2016 15:58:53 +0100 Subject: [PATCH 20/28] core: be strict about connection argument in D-Bus methods There is no excuse for clients to send connections to NetworkManager that have invalid/unknown fields. Just reject them. This is a dangerous change, because we might now reject connections that we were accepting previously. Who know what clients were sending and it used to work. --- src/settings/nm-settings-connection.c | 3 ++- src/settings/nm-settings.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index bce3dff104..6bed3fe5f2 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -1736,7 +1736,8 @@ settings_connection_update_helper (NMSettingsConnection *self, /* Check if the settings are valid first */ if (new_settings) { tmp = _nm_simple_connection_new_from_dbus (new_settings, - NM_SETTING_PARSE_FLAGS_NORMALIZE, + NM_SETTING_PARSE_FLAGS_STRICT + | NM_SETTING_PARSE_FLAGS_NORMALIZE, &error); if (!tmp) goto error; diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index fc2daa063c..77e45f4982 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1408,7 +1408,8 @@ impl_settings_add_connection_helper (NMSettings *self, GError *error = NULL; connection = _nm_simple_connection_new_from_dbus (settings, - NM_SETTING_PARSE_FLAGS_NORMALIZE, + NM_SETTING_PARSE_FLAGS_STRICT + | NM_SETTING_PARSE_FLAGS_NORMALIZE, &error); if (connection) { From 52bd08de051e811c8b0aea354ae448e4647dfd0d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Mar 2016 17:36:28 +0100 Subject: [PATCH 21/28] libnm: add code comments to hint that NMConnection might not validate --- libnm/nm-client.c | 12 ++++++++++++ libnm/nm-device.c | 6 ++++++ 2 files changed, 18 insertions(+) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index 57a43ca8dc..a7ecdb5572 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -1189,6 +1189,9 @@ nm_client_deactivate_connection_finish (NMClient *client, * Returns: (transfer none) (element-type NMRemoteConnection): an array * containing all connections provided by the remote settings service. The * returned array is owned by the #NMClient object and should not be modified. + * + * The connections are as received from D-Bus and might not validate according + * to nm_connection_verify(). **/ const GPtrArray * nm_client_get_connections (NMClient *client) @@ -1207,6 +1210,9 @@ nm_client_get_connections (NMClient *client) * * Returns: (transfer none): the remote connection object on success, or %NULL if no * matching object was found. + * + * The connection is as received from D-Bus and might not validate according + * to nm_connection_verify(). **/ NMRemoteConnection * nm_client_get_connection_by_id (NMClient *client, const char *id) @@ -1226,6 +1232,9 @@ nm_client_get_connection_by_id (NMClient *client, const char *id) * * Returns: (transfer none): the remote connection object on success, or %NULL if the object was * not known + * + * The connection is as received from D-Bus and might not validate according + * to nm_connection_verify(). **/ NMRemoteConnection * nm_client_get_connection_by_path (NMClient *client, const char *path) @@ -1245,6 +1254,9 @@ nm_client_get_connection_by_path (NMClient *client, const char *path) * * Returns: (transfer none): the remote connection object on success, or %NULL if the object was * not known + * + * The connection is as received from D-Bus and might not validate according + * to nm_connection_verify(). **/ NMRemoteConnection * nm_client_get_connection_by_uuid (NMClient *client, const char *uuid) diff --git a/libnm/nm-device.c b/libnm/nm-device.c index 5679fd5f3d..496dd87a06 100644 --- a/libnm/nm-device.c +++ b/libnm/nm-device.c @@ -2329,6 +2329,9 @@ nm_device_reapply_finish (NMDevice *device, * Returns: (transfer full): a %NMConnection with the currently applied settings * or %NULL on error. * + * The connection is as received from D-Bus and might not validate according + * to nm_connection_verify(). + * * Since: 1.2 **/ NMConnection * @@ -2457,6 +2460,9 @@ nm_device_get_applied_connection_async (NMDevice *device, * Returns: (transfer full): a currently applied %NMConnection or %NULL in case * of error. * + * The connection is as received from D-Bus and might not validate according + * to nm_connection_verify(). + * * Since: 1.2 **/ NMConnection * From 564cf78f69520e1694b0ca05e5ad9d8b2fba18f2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 24 Mar 2016 16:03:06 +0100 Subject: [PATCH 22/28] libnm-util: use "nm-shared-utils.h" --- libnm-util/Makefile.am | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libnm-util/Makefile.am b/libnm-util/Makefile.am index 6e63790817..e9ccdae7f3 100644 --- a/libnm-util/Makefile.am +++ b/libnm-util/Makefile.am @@ -63,6 +63,7 @@ nodist_libnm_util_include_HEADERS = \ nm-utils-enum-types.h libnm_util_la_private_headers = \ + $(top_builddir)/shared/nm-shared-utils.h \ crypto.h \ nm-dbus-glib-types.h \ nm-gvaluearray-compat.h \ @@ -71,6 +72,7 @@ libnm_util_la_private_headers = \ nm-utils-private.h libnm_util_la_csources = \ + $(top_builddir)/shared/nm-shared-utils.c \ crypto.c \ nm-connection.c \ nm-param-spec-specialized.c \ From f8a6b13369cdaf73d28e069e00b6185967eca5ad Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 24 Mar 2016 15:58:28 +0100 Subject: [PATCH 23/28] libnm-util: don't print any warnings during nm_setting_new_from_hash() Warnings aren't great, especially if they can realistically be triggered by a newer NetworkManager version. Just accept what we can and ignore the rest silently. --- libnm-util/nm-setting.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/libnm-util/nm-setting.c b/libnm-util/nm-setting.c index add3ad9795..990d6fe73b 100644 --- a/libnm-util/nm-setting.c +++ b/libnm-util/nm-setting.c @@ -379,9 +379,6 @@ nm_setting_new_from_hash (GType setting_type, GHashTable *hash) const char *prop_name; GValue *src_value; GObjectClass *class; - guint n_params = 0; - GParameter *params; - int i; g_return_val_if_fail (G_TYPE_IS_INSTANTIATABLE (setting_type), NULL); g_return_val_if_fail (hash != NULL, NULL); @@ -390,11 +387,11 @@ nm_setting_new_from_hash (GType setting_type, GHashTable *hash) * already been used. */ class = g_type_class_ref (setting_type); - params = g_new0 (GParameter, g_hash_table_size (hash)); + + setting = (NMSetting *) g_object_new (setting_type, NULL); g_hash_table_iter_init (&iter, hash); while (g_hash_table_iter_next (&iter, (gpointer) &prop_name, (gpointer) &src_value)) { - GValue *dst_value = ¶ms[n_params].value; GParamSpec *param_spec; param_spec = g_object_class_find_property (class, prop_name); @@ -402,21 +399,12 @@ nm_setting_new_from_hash (GType setting_type, GHashTable *hash) /* Assume that any unrecognized property either can be ignored, or * else has a backward-compatibility equivalent. */ - g_debug ("Ignoring unrecognized property '%s'", prop_name); continue; } - g_value_init (dst_value, G_VALUE_TYPE (src_value)); - g_value_copy (src_value, dst_value); - params[n_params++].name = prop_name; + nm_g_object_set_property ((GObject *) setting, prop_name, src_value, NULL); } - setting = (NMSetting *) g_object_newv (setting_type, n_params, params); - - for (i = 0; i < n_params; i++) - g_value_unset (¶ms[i].value); - - g_free (params); g_type_class_unref (class); return setting; From 6cd03bf205745bf13ec3a9af7af30a376e348015 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 26 Mar 2016 12:26:54 +0100 Subject: [PATCH 24/28] libnm-util: refactor hash_to_connection() No functional change, only move the verify-step out of hash_to_connection(). --- libnm-util/nm-connection.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/libnm-util/nm-connection.c b/libnm-util/nm-connection.c index daeee46b7e..f77e322307 100644 --- a/libnm-util/nm-connection.c +++ b/libnm-util/nm-connection.c @@ -327,13 +327,13 @@ validate_permissions_type (GHashTable *hash, GError **error) return TRUE; } -static gboolean -hash_to_connection (NMConnection *connection, GHashTable *new, GError **error) +static void +hash_to_connection (NMConnection *connection, GHashTable *new) { GHashTableIter iter; const char *setting_name; GHashTable *setting_hash; - gboolean changed, valid; + gboolean changed; NMConnectionPrivate *priv = NM_CONNECTION_GET_PRIVATE (connection); if ((changed = g_hash_table_size (priv->settings) > 0)) @@ -353,10 +353,8 @@ hash_to_connection (NMConnection *connection, GHashTable *new, GError **error) } } - valid = nm_connection_verify (connection, error); if (changed) g_signal_emit (connection, signals[CHANGED], 0); - return valid; } /** @@ -373,16 +371,16 @@ nm_connection_replace_settings (NMConnection *connection, GHashTable *new_settings, GError **error) { - gboolean valid = FALSE; - g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE); g_return_val_if_fail (new_settings != NULL, FALSE); if (error) g_return_val_if_fail (*error == NULL, FALSE); - if (validate_permissions_type (new_settings, error)) - valid = hash_to_connection (connection, new_settings, error); - return valid; + if (!validate_permissions_type (new_settings, error)) + return FALSE; + + hash_to_connection (connection, new_settings); + return nm_connection_verify (connection, error); } /** @@ -1466,10 +1464,9 @@ nm_connection_new_from_hash (GHashTable *hash, GError **error) return NULL; connection = nm_connection_new (); - if (!hash_to_connection (connection, hash, error)) { - g_object_unref (connection); - return NULL; - } + hash_to_connection (connection, hash); + if (!nm_connection_verify (connection, error)) + g_clear_object (&connection); return connection; } From 9bf3933855f6c9ccd65447e73a59e24055a6e340 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 26 Mar 2016 13:04:43 +0100 Subject: [PATCH 25/28] libnm-util: add non-failing versions of nm_connection_new_from_hash() and replace-settings Add internal functions _nm_connection_replace_settings() and _nm_connection_new_from_hash() that cannot fail. Altough they are not public API, we have to expose them via libnm-util.ver so that they can be used from libnm-glib. --- libnm-util/libnm-util.ver | 2 ++ libnm-util/nm-connection.c | 50 +++++++++++++++++++++++++++------ libnm-util/nm-setting-private.h | 6 ++++ 3 files changed, 49 insertions(+), 9 deletions(-) diff --git a/libnm-util/libnm-util.ver b/libnm-util/libnm-util.ver index 1ad853f5fb..385cdb8e3b 100644 --- a/libnm-util/libnm-util.ver +++ b/libnm-util/libnm-util.ver @@ -1,5 +1,7 @@ { global: + _nm_connection_new_from_hash; + _nm_connection_replace_settings; nm_connection_add_setting; nm_connection_clear_secrets; nm_connection_clear_secrets_with_flags; diff --git a/libnm-util/nm-connection.c b/libnm-util/nm-connection.c index f77e322307..51c480284f 100644 --- a/libnm-util/nm-connection.c +++ b/libnm-util/nm-connection.c @@ -327,19 +327,30 @@ validate_permissions_type (GHashTable *hash, GError **error) return TRUE; } -static void -hash_to_connection (NMConnection *connection, GHashTable *new) +/** + * _nm_connection_replace_settings: + * @connection: a #NMConnection + * @new_settings: (element-type utf8 GLib.HashTable): a #GHashTable of settings + **/ +void +_nm_connection_replace_settings (NMConnection *connection, + GHashTable *new_settings) { + NMConnectionPrivate *priv = NM_CONNECTION_GET_PRIVATE (connection); GHashTableIter iter; const char *setting_name; GHashTable *setting_hash; gboolean changed; - NMConnectionPrivate *priv = NM_CONNECTION_GET_PRIVATE (connection); + + g_return_if_fail (NM_IS_CONNECTION (connection)); + g_return_if_fail (new_settings != NULL); + + priv = NM_CONNECTION_GET_PRIVATE (connection); if ((changed = g_hash_table_size (priv->settings) > 0)) g_hash_table_foreach_remove (priv->settings, _setting_release, connection); - g_hash_table_iter_init (&iter, new); + g_hash_table_iter_init (&iter, new_settings); while (g_hash_table_iter_next (&iter, (gpointer) &setting_name, (gpointer) &setting_hash)) { GType type = nm_connection_lookup_setting_type (setting_name); @@ -373,13 +384,12 @@ nm_connection_replace_settings (NMConnection *connection, { g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE); g_return_val_if_fail (new_settings != NULL, FALSE); - if (error) - g_return_val_if_fail (*error == NULL, FALSE); + g_return_val_if_fail (!error || !*error, FALSE); if (!validate_permissions_type (new_settings, error)) return FALSE; - hash_to_connection (connection, new_settings); + _nm_connection_replace_settings (connection, new_settings); return nm_connection_verify (connection, error); } @@ -1440,6 +1450,29 @@ nm_connection_new (void) return (NMConnection *) g_object_new (NM_TYPE_CONNECTION, NULL); } +/** + * _nm_connection_new_from_hash: + * @hash: (element-type utf8 GLib.HashTable): the #GHashTable describing + * the connection + * + * Creates a new #NMConnection from a hash table describing the connection. See + * nm_connection_to_hash() for a description of the expected hash table. + * + * Returns: the new #NMConnection object, populated with settings created + * from the values in the hash table. + **/ +NMConnection * +_nm_connection_new_from_hash (GHashTable *hash) +{ + NMConnection *connection; + + g_return_val_if_fail (hash != NULL, NULL); + + connection = nm_connection_new (); + _nm_connection_replace_settings (connection, hash); + return connection; +} + /** * nm_connection_new_from_hash: * @hash: (element-type utf8 GLib.HashTable): the #GHashTable describing @@ -1463,8 +1496,7 @@ nm_connection_new_from_hash (GHashTable *hash, GError **error) if (!validate_permissions_type (hash, error)) return NULL; - connection = nm_connection_new (); - hash_to_connection (connection, hash); + connection = _nm_connection_new_from_hash (hash); if (!nm_connection_verify (connection, error)) g_clear_object (&connection); return connection; diff --git a/libnm-util/nm-setting-private.h b/libnm-util/nm-setting-private.h index 6956463c3f..beb87484f0 100644 --- a/libnm-util/nm-setting-private.h +++ b/libnm-util/nm-setting-private.h @@ -23,6 +23,8 @@ #include "nm-default.h" +#include "nm-connection.h" + #define NM_SETTING_SECRET_FLAGS_ALL \ (NM_SETTING_SECRET_FLAG_NONE | \ NM_SETTING_SECRET_FLAG_AGENT_OWNED | \ @@ -62,6 +64,10 @@ gint _nm_setting_compare_priority (gconstpointer a, gconstpointer b); gboolean _nm_setting_get_property (NMSetting *setting, const char *name, GValue *value); +NMConnection *_nm_connection_new_from_hash (GHashTable *hash); +void _nm_connection_replace_settings (NMConnection *connection, + GHashTable *new_settings); + typedef enum NMSettingUpdateSecretResult { NM_SETTING_UPDATE_SECRET_ERROR = FALSE, NM_SETTING_UPDATE_SECRET_SUCCESS_MODIFIED = TRUE, From fbb1662269e3c47fa51abfe37d1f3c8f0a213bd0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 26 Mar 2016 13:18:30 +0100 Subject: [PATCH 26/28] libnm-glib: don't fail creating connection in NMSecretAgent The connection should be created best-effort. If the connection doesn't validate, the connection request still can be answered by the agent. --- libnm-glib/nm-secret-agent.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/libnm-glib/nm-secret-agent.c b/libnm-glib/nm-secret-agent.c index 0a86245568..03290469ad 100644 --- a/libnm-glib/nm-secret-agent.c +++ b/libnm-glib/nm-secret-agent.c @@ -27,6 +27,7 @@ #include "nm-secret-agent.h" #include "nm-glib-enum-types.h" #include "nm-dbus-helpers-private.h" +#include "nm-setting-private.h" static void impl_secret_agent_get_secrets (NMSecretAgent *self, GHashTable *connection_hash, @@ -302,7 +303,8 @@ verify_request (NMSecretAgent *self, GError **error) { NMConnection *connection = NULL; - GError *local = NULL; + + g_return_val_if_fail (out_connection, FALSE); if (!verify_sender (self, context, error)) return FALSE; @@ -321,21 +323,11 @@ verify_request (NMSecretAgent *self, } /* Make sure the given connection is valid */ - g_assert (out_connection); - connection = nm_connection_new_from_hash (connection_hash, &local); - if (connection) { - nm_connection_set_path (connection, connection_path); - *out_connection = connection; - } else { - g_set_error (error, - NM_SECRET_AGENT_ERROR, - NM_SECRET_AGENT_ERROR_INVALID_CONNECTION, - "Invalid connection: %s", - local->message); - g_clear_error (&local); - } + connection = _nm_connection_new_from_hash (connection_hash); + nm_connection_set_path (connection, connection_path); + *out_connection = connection; - return !!connection; + return TRUE; } static void From 0578be928b70eafaba88ef98a2e65c3712fda2d2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 26 Mar 2016 13:18:30 +0100 Subject: [PATCH 27/28] libnm-glib: don't fail creating connection in NMVPNPlugin The connection should be created best-effort. If the connection doesn't validate, the request can still make sense for the plugin. --- libnm-glib/nm-vpn-plugin.c | 32 ++++---------------------------- 1 file changed, 4 insertions(+), 28 deletions(-) diff --git a/libnm-glib/nm-vpn-plugin.c b/libnm-glib/nm-vpn-plugin.c index d117e53092..2af9deb4bd 100644 --- a/libnm-glib/nm-vpn-plugin.c +++ b/libnm-glib/nm-vpn-plugin.c @@ -29,6 +29,7 @@ #include "nm-utils.h" #include "nm-connection.h" #include "nm-dbus-glib-types.h" +#include "nm-setting-private.h" static gboolean impl_vpn_plugin_connect (NMVPNPlugin *plugin, GHashTable *connection, @@ -452,13 +453,7 @@ _connect_generic (NMVPNPlugin *plugin, return FALSE; } - connection = nm_connection_new_from_hash (properties, &local); - if (!connection) { - g_set_error (error, NM_VPN_PLUGIN_ERROR, NM_VPN_PLUGIN_ERROR_BAD_ARGUMENTS, - "Invalid connection: %s", local->message); - g_clear_error (&local); - return FALSE; - } + connection = _nm_connection_new_from_hash (properties); priv->interactive = FALSE; if (details && !vpn_class->connect_interactive) { @@ -526,22 +521,11 @@ impl_vpn_plugin_need_secrets (NMVPNPlugin *plugin, char *sn = NULL; GError *ns_err = NULL; gboolean needed = FALSE; - GError *cnfh_err = NULL; g_return_val_if_fail (NM_IS_VPN_PLUGIN (plugin), FALSE); g_return_val_if_fail (properties != NULL, FALSE); - connection = nm_connection_new_from_hash (properties, &cnfh_err); - if (!connection) { - g_set_error (err, - NM_VPN_PLUGIN_ERROR, - NM_VPN_PLUGIN_ERROR_CONNECTION_INVALID, - "The connection was invalid: '%s' / '%s' invalid.", - g_type_name (nm_connection_lookup_setting_type_by_quark (cnfh_err->domain)), - cnfh_err->message); - g_error_free (cnfh_err); - return FALSE; - } + connection = _nm_connection_new_from_hash (properties); if (!NM_VPN_PLUGIN_GET_CLASS (plugin)->need_secrets) { *setting_name = ""; @@ -581,7 +565,6 @@ impl_vpn_plugin_new_secrets (NMVPNPlugin *plugin, { NMVPNPluginPrivate *priv = NM_VPN_PLUGIN_GET_PRIVATE (plugin); NMConnection *connection; - GError *local = NULL; gboolean success; if (priv->state != NM_VPN_SERVICE_STATE_STARTING) { @@ -591,14 +574,7 @@ impl_vpn_plugin_new_secrets (NMVPNPlugin *plugin, return FALSE; } - connection = nm_connection_new_from_hash (properties, &local); - if (!connection) { - g_set_error (error, NM_VPN_PLUGIN_ERROR, NM_VPN_PLUGIN_ERROR_BAD_ARGUMENTS, - "Invalid connection: %s", - local->message); - g_clear_error (&local); - return FALSE; - } + connection = _nm_connection_new_from_hash (properties); if (!NM_VPN_PLUGIN_GET_CLASS (plugin)->new_secrets) { g_set_error_literal (error, NM_VPN_PLUGIN_ERROR, NM_VPN_PLUGIN_ERROR_INTERACTIVE_NOT_SUPPORTED, From 1c932aade180d05b0f75f1268efabdef8eac1d69 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 26 Mar 2016 13:47:30 +0100 Subject: [PATCH 28/28] libnm-glib: allow non-verifiable NMRemoteConnection in libnm-glib --- libnm-glib/nm-remote-connection.c | 30 ++++++++-------------------- libnm-glib/tests/test-nm-client.c | 33 +++++++++++-------------------- 2 files changed, 20 insertions(+), 43 deletions(-) diff --git a/libnm-glib/nm-remote-connection.c b/libnm-glib/nm-remote-connection.c index 132032f488..a4dc638adc 100644 --- a/libnm-glib/nm-remote-connection.c +++ b/libnm-glib/nm-remote-connection.c @@ -31,6 +31,7 @@ #include "nm-object-private.h" #include "nm-dbus-glib-types.h" #include "nm-dbus-helpers-private.h" +#include "nm-setting-private.h" #define NM_REMOTE_CONNECTION_BUS "bus" #define NM_REMOTE_CONNECTION_DBUS_CONNECTION "dbus-connection" @@ -446,24 +447,6 @@ nm_remote_connection_get_unsaved (NMRemoteConnection *connection) /****************************************************************/ -static void -replace_settings (NMRemoteConnection *self, GHashTable *new_settings) -{ - GError *error = NULL; - - if (nm_connection_replace_settings (NM_CONNECTION (self), new_settings, &error)) - g_signal_emit (self, signals[UPDATED], 0, new_settings); - else { - g_warning ("%s: error updating connection %s settings: %s", - __func__, - nm_connection_get_path (NM_CONNECTION (self)), - error->message); - g_clear_error (&error); - - g_signal_emit (self, signals[REMOVED], 0); - } -} - static void updated_get_settings_cb (DBusGProxy *proxy, DBusGProxyCall *call, @@ -488,7 +471,7 @@ updated_get_settings_cb (DBusGProxy *proxy, * object. */ hash = g_hash_table_new (g_str_hash, g_str_equal); - nm_connection_replace_settings (NM_CONNECTION (self), hash, NULL); + _nm_connection_replace_settings (NM_CONNECTION (self), hash); g_hash_table_destroy (hash); priv->visible = FALSE; @@ -497,7 +480,8 @@ updated_get_settings_cb (DBusGProxy *proxy, gs_unref_object NMConnection *self_alive = NULL; self_alive = g_object_ref (self); - replace_settings (self, new_settings); + _nm_connection_replace_settings (NM_CONNECTION (self), new_settings); + g_signal_emit (self, signals[UPDATED], 0, new_settings); g_hash_table_destroy (new_settings); /* Settings service will handle announcing the connection to clients */ @@ -628,7 +612,8 @@ init_sync (GInitable *initable, GCancellable *cancellable, GError **error) return FALSE; priv->visible = TRUE; self_alive = g_object_ref (initable); - replace_settings (NM_REMOTE_CONNECTION (initable), hash); + _nm_connection_replace_settings (NM_CONNECTION (initable), hash); + g_signal_emit (initable, signals[UPDATED], 0, hash); g_hash_table_destroy (hash); /* Get properties */ @@ -703,7 +688,8 @@ init_get_settings_cb (DBusGProxy *proxy, priv->visible = TRUE; self_alive = g_object_ref (init_data->connection); - replace_settings (init_data->connection, settings); + _nm_connection_replace_settings (NM_CONNECTION (init_data->connection), settings); + g_signal_emit (init_data->connection, signals[UPDATED], 0, settings); g_hash_table_destroy (settings); /* Grab properties */ diff --git a/libnm-glib/tests/test-nm-client.c b/libnm-glib/tests/test-nm-client.c index da2e39e04f..0da93c3fd3 100644 --- a/libnm-glib/tests/test-nm-client.c +++ b/libnm-glib/tests/test-nm-client.c @@ -955,12 +955,8 @@ test_connection_invalid (void) settings = nmtstc_nm_remote_settings_new (); - g_test_expect_message ("libnm-glib", G_LOG_LEVEL_WARNING, "*replace_settings: error updating connection*"); - nmtst_main_loop_run (loop, 100); - g_test_assert_expected_messages (); - _slist_to_array (&connections, nm_remote_settings_list_connections (settings)); g_assert_cmpint (connections->len, ==, 2); @@ -991,12 +987,8 @@ test_connection_invalid (void) FALSE, &path2); - g_test_expect_message ("libnm-glib", G_LOG_LEVEL_WARNING, "*replace_settings: error updating connection*"); - nmtst_main_loop_run (loop, 100); - g_test_assert_expected_messages (); - _slist_to_array (&connections, nm_remote_settings_list_connections (settings)); g_assert_cmpint (connections->len, ==, 3); @@ -1027,15 +1019,11 @@ test_connection_invalid (void) connection, FALSE); - g_test_expect_message ("libnm-glib", G_LOG_LEVEL_WARNING, "*replace_settings: error updating connection*"); - nmtst_main_loop_run (loop, 100); - g_test_assert_expected_messages (); - _slist_to_array (&connections, nm_remote_settings_list_connections (settings)); - g_assert_cmpint (connections->len, ==, 2); + g_assert_cmpint (connections->len, ==, 3); n_found = nmtst_find_all_indexes (connections->pdata, connections->len, (gpointer *) ((const char *[]) { path0, path1, path2 }), @@ -1043,12 +1031,13 @@ test_connection_invalid (void) _test_connection_invalid_find_connections, NULL, idx); - g_assert_cmpint (n_found, ==, 2); + g_assert_cmpint (n_found, ==, 3); ASSERT_IDX (0); ASSERT_IDX (1); - g_assert_cmpint (idx[2], ==, -1); + ASSERT_IDX (2); nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[0]]); nmtst_assert_connection_unnormalizable (connections->pdata[idx[1]], 0, 0); + nmtst_assert_connection_unnormalizable (connections->pdata[idx[2]], 0, 0); /************************************************************************** * Modify the invalid connection again. Note that the connection stays @@ -1073,7 +1062,7 @@ test_connection_invalid (void) _slist_to_array (&connections, nm_remote_settings_list_connections (settings)); - g_assert_cmpint (connections->len, ==, 2); + g_assert_cmpint (connections->len, ==, 3); n_found = nmtst_find_all_indexes (connections->pdata, connections->len, (gpointer *) ((const char *[]) { path0, path1, path2 }), @@ -1081,12 +1070,13 @@ test_connection_invalid (void) _test_connection_invalid_find_connections, NULL, idx); - g_assert_cmpint (n_found, ==, 2); + g_assert_cmpint (n_found, ==, 3); ASSERT_IDX (0); ASSERT_IDX (1); - g_assert_cmpint (idx[2], ==, -1); + ASSERT_IDX (2); nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[0]]); nmtst_assert_connection_unnormalizable (connections->pdata[idx[1]], 0, 0); + nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[2]]); /************************************************************************** @@ -1111,7 +1101,7 @@ test_connection_invalid (void) _slist_to_array (&connections, nm_remote_settings_list_connections (settings)); - g_assert_cmpint (connections->len, ==, 2); + g_assert_cmpint (connections->len, ==, 3); n_found = nmtst_find_all_indexes (connections->pdata, connections->len, (gpointer *) ((const char *[]) { path0, path1, path2 }), @@ -1119,12 +1109,13 @@ test_connection_invalid (void) _test_connection_invalid_find_connections, NULL, idx); - g_assert_cmpint (n_found, ==, 2); + g_assert_cmpint (n_found, ==, 3); ASSERT_IDX (0); ASSERT_IDX (1); - g_assert_cmpint (idx[2], ==, -1); + ASSERT_IDX (2); nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[0]]); nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[1]]); + nmtst_assert_connection_verifies_without_normalization (connections->pdata[idx[2]]); g_assert_cmpstr ("test-connection-invalid-1x", ==, nm_connection_get_id (connections->pdata[idx[1]])); #undef ASSERT_IDX