From 0b6a9e2c887b89b578a530aaaafd8f440169a7a6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 26 Sep 2022 16:30:25 +0200 Subject: [PATCH 1/3] glib-aux: add nm_ref_string_reset() helper --- src/libnm-glib-aux/nm-ref-string.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/libnm-glib-aux/nm-ref-string.h b/src/libnm-glib-aux/nm-ref-string.h index eb3c38de21..0cfea5fae3 100644 --- a/src/libnm-glib-aux/nm-ref-string.h +++ b/src/libnm-glib-aux/nm-ref-string.h @@ -93,6 +93,23 @@ nm_ref_string_unref(NMRefString *rstr) NM_AUTO_DEFINE_FCN_VOID(NMRefString *, _nm_auto_ref_string, nm_ref_string_unref); #define nm_auto_ref_string nm_auto(_nm_auto_ref_string) +static inline gboolean +nm_ref_string_reset(NMRefString **ptr, NMRefString *str) +{ + NMRefString *rstr; + + nm_assert(ptr); + + rstr = *ptr; + + if (rstr == str) + return FALSE; + + *ptr = nm_ref_string_ref(str); + nm_ref_string_unref(rstr); + return TRUE; +} + /*****************************************************************************/ static inline const char * From 8ab23e5b2d8c3dc87581ce2e090886cf0925bb5d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 26 Sep 2022 16:47:56 +0200 Subject: [PATCH 2/3] glib-aux: reorder comparison in nm_ref_string_equal_str() We usually compare first for pointer equality. It seems to make more sense this way. Swap. --- src/libnm-glib-aux/nm-ref-string.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-ref-string.h b/src/libnm-glib-aux/nm-ref-string.h index 0cfea5fae3..1b0cabf26d 100644 --- a/src/libnm-glib-aux/nm-ref-string.h +++ b/src/libnm-glib-aux/nm-ref-string.h @@ -156,7 +156,7 @@ nm_ref_string_equal_str(NMRefString *rstr, const char *str) /* We don't use streq() here, because an NMRefString might have embedded NUL characters * (as the length is tracked separately). The NUL terminated C string @str must not * compare equal to such a @rstr, thus we first explicitly check strlen(). */ - return rstr->len == strlen(str) && (rstr->str == str || memcmp(rstr->str, str, rstr->len) == 0); + return rstr->str == str || (rstr->len == strlen(str) && memcmp(rstr->str, str, rstr->len) == 0); } static inline gboolean From a1ab9d9e1c8137867bae78671f449e6361377d03 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 26 Sep 2022 13:39:23 +0200 Subject: [PATCH 3/3] libnm: use NMRefString for nm_connection_get_path() NMConnection is an interface, implemented by NMSimpleConnection and NMRemoteConnection. For the most part, an NMConnection is only the content of the profile (the settings). The "path" of the connection refers to the D-Bus path, and wouldn't really make sense of the NMConnection interface or the NMSimpleConnection type. As such, the daemon (which only uses NMConnection and NMSimpleConnection) never sets the path. Only libnm does. NMClient uses NMRefString extensively for the D-Bus interface and the path is already internalized. Take advantage of that. It is very likely, that we are able to share the path instance in libnm at which point it makes sense to use NMRefString. Also, during nm_simple_connection_new_clone(), we can just take another reference instead of cloning the string. --- src/libnm-client-impl/nm-remote-connection.c | 2 +- src/libnm-core-impl/nm-connection.c | 20 +++++++---- src/libnm-core-impl/nm-setting-private.h | 4 ++- src/libnm-core-impl/nm-simple-connection.c | 5 +-- src/libnm-core-impl/tests/test-general.c | 38 ++++++++++++++++++++ src/libnm-core-intern/nm-core-internal.h | 8 +++++ 6 files changed, 65 insertions(+), 12 deletions(-) diff --git a/src/libnm-client-impl/nm-remote-connection.c b/src/libnm-client-impl/nm-remote-connection.c index 23dfc1dac3..0810e540dc 100644 --- a/src/libnm-client-impl/nm-remote-connection.c +++ b/src/libnm-client-impl/nm-remote-connection.c @@ -696,7 +696,7 @@ static void register_client(NMObject *nmobj, NMClient *client, NMLDBusObject *dbobj) { NM_OBJECT_CLASS(nm_remote_connection_parent_class)->register_client(nmobj, client, dbobj); - nm_connection_set_path(NM_CONNECTION(nmobj), dbobj->dbus_path->str); + _nm_connection_set_path_rstr(NM_CONNECTION(nmobj), dbobj->dbus_path); _nm_client_get_settings_call(client, dbobj); } diff --git a/src/libnm-core-impl/nm-connection.c b/src/libnm-core-impl/nm-connection.c index 60e7eb7ff2..6c73c66bb5 100644 --- a/src/libnm-core-impl/nm-connection.c +++ b/src/libnm-core-impl/nm-connection.c @@ -11,6 +11,7 @@ #include #include "libnm-glib-aux/nm-uuid.h" +#include "libnm-glib-aux/nm-ref-string.h" #include "nm-connection-private.h" #include "nm-utils.h" #include "nm-setting-private.h" @@ -59,7 +60,7 @@ _nm_connection_private_clear(NMConnectionPrivate *priv) { if (priv->self) { _nm_connection_clear_settings(priv->self, priv); - nm_clear_g_free(&priv->path); + nm_clear_pointer(&priv->path, nm_ref_string_unref); priv->self = NULL; } } @@ -2913,14 +2914,15 @@ nm_connection_dump(NMConnection *connection) void nm_connection_set_path(NMConnection *connection, const char *path) { - NMConnectionPrivate *priv; - g_return_if_fail(NM_IS_CONNECTION(connection)); - priv = NM_CONNECTION_GET_PRIVATE(connection); + nm_ref_string_reset_str(&NM_CONNECTION_GET_PRIVATE(connection)->path, path); +} - g_free(priv->path); - priv->path = g_strdup(path); +void +_nm_connection_set_path_rstr(NMConnection *connection, NMRefString *path) +{ + nm_ref_string_reset(&NM_CONNECTION_GET_PRIVATE(connection)->path, path); } /** @@ -2937,6 +2939,12 @@ nm_connection_get_path(NMConnection *connection) { g_return_val_if_fail(NM_IS_CONNECTION(connection), NULL); + return nm_ref_string_get_str(NM_CONNECTION_GET_PRIVATE(connection)->path); +} + +NMRefString * +_nm_connection_get_path_rstr(NMConnection *connection) +{ return NM_CONNECTION_GET_PRIVATE(connection)->path; } diff --git a/src/libnm-core-impl/nm-setting-private.h b/src/libnm-core-impl/nm-setting-private.h index 005d04808a..221a527e12 100644 --- a/src/libnm-core-impl/nm-setting-private.h +++ b/src/libnm-core-impl/nm-setting-private.h @@ -20,13 +20,15 @@ /*****************************************************************************/ +struct _NMRefString; + typedef struct { NMConnection *self; NMSetting *settings[_NM_META_SETTING_TYPE_NUM]; /* D-Bus path of the connection, if any */ - char *path; + struct _NMRefString *path; } NMConnectionPrivate; extern GTypeClass *_nm_simple_connection_class_instance; diff --git a/src/libnm-core-impl/nm-simple-connection.c b/src/libnm-core-impl/nm-simple-connection.c index 802be3caa4..a9c66dc72d 100644 --- a/src/libnm-core-impl/nm-simple-connection.c +++ b/src/libnm-core-impl/nm-simple-connection.c @@ -136,15 +136,12 @@ NMConnection * nm_simple_connection_new_clone(NMConnection *connection) { NMConnection *clone; - const char *path; g_return_val_if_fail(NM_IS_CONNECTION(connection), NULL); clone = nm_simple_connection_new(); - path = nm_connection_get_path(connection); - if (path) - nm_connection_set_path(clone, path); + _nm_connection_set_path_rstr(clone, _nm_connection_get_path_rstr(connection)); nm_connection_replace_settings_from_connection(clone, connection); diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c index d66b820d7f..904d89f337 100644 --- a/src/libnm-core-impl/tests/test-general.c +++ b/src/libnm-core-impl/tests/test-general.c @@ -11081,6 +11081,43 @@ test_direct_string_is_refstr(void) /*****************************************************************************/ +static void +test_connection_path(void) +{ + gs_unref_object NMConnection *conn = NULL; + const char *const PATH = "/org/freedesktop/NetworkManager/Settings/171950003017"; + const char *path; + NMRefString *rstr; + + g_assert(!nmtst_ref_string_find(PATH)); + + conn = nmtst_create_minimal_connection("test_setting_ip6_gateway", + NULL, + NM_SETTING_WIRED_SETTING_NAME, + NULL); + + g_assert(!nm_connection_get_path(conn)); + g_assert(!nmtst_ref_string_find(PATH)); + + nm_connection_set_path(conn, PATH); + + path = nm_connection_get_path(conn); + g_assert_cmpstr(path, ==, PATH); + + /* nm_connection_get_path() gives a NMRefString. This is an + * implementation detail, but libnm (which statically links with + * libnm-core) may choose to rely on that. */ + rstr = nmtst_ref_string_find(PATH); + g_assert(rstr); + g_assert(NM_REF_STRING_UPCAST(path) == rstr); + + g_clear_object(&conn); + + g_assert(!nmtst_ref_string_find(PATH)); +} + +/*****************************************************************************/ + NMTST_DEFINE(); int @@ -11426,6 +11463,7 @@ main(int argc, char **argv) g_test_add_func("/core/general/test_system_encodings", test_system_encodings); g_test_add_func("/core/general/test_direct_string_is_refstr", test_direct_string_is_refstr); + g_test_add_func("/core/general/test_connection_path", test_connection_path); return g_test_run(); } diff --git a/src/libnm-core-intern/nm-core-internal.h b/src/libnm-core-intern/nm-core-internal.h index 4e1bab4723..9e9a673a10 100644 --- a/src/libnm-core-intern/nm-core-internal.h +++ b/src/libnm-core-intern/nm-core-internal.h @@ -481,6 +481,14 @@ NMSettingIPConfig *nm_connection_get_setting_ip_config(NMConnection *connection, /*****************************************************************************/ +struct _NMRefString; + +void _nm_connection_set_path_rstr(NMConnection *connection, struct _NMRefString *path); + +struct _NMRefString *_nm_connection_get_path_rstr(NMConnection *connection); + +/*****************************************************************************/ + typedef enum { NM_BOND_OPTION_TYPE_INT, NM_BOND_OPTION_TYPE_BOTH,