From ccf6bdb0e2c9fa1ccac4bb1c482ca967eac9dbe9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 15 Jul 2018 07:25:36 +0200 Subject: [PATCH 1/3] shared: add nm_gobject_notify_together() helper NM_GOBJECT_PROPERTIES_DEFINE() defines a helper function _notify() to emit a GObject property changed notification. Add another helper function to emit multiple notifications together, and freeze/thaw the notification before. This is particularly useful, because our D-Bus glue in "nm-dbus-object.c" hooks into dispatch_properties_changed(), to emit a combined PropertiesChanged signal for multiple properties. By carefully freezing/thawing the notifications, the exported objects can combine changes of multiple properties in one D-Bus signal. This helper is here to make that simpler. Note that the compiler still has no problem to inline _notify() entirey. So, in a non-debug build, there is little difference in the generated code. It can even nicely inline calls like nm_gobject_notify_together (self, PROP_ADDRESS_DATA, PROP_ADDRESSES); --- shared/nm-utils/nm-macros-internal.h | 32 ++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/shared/nm-utils/nm-macros-internal.h b/shared/nm-utils/nm-macros-internal.h index 2ff95a288..4879e33d0 100644 --- a/shared/nm-utils/nm-macros-internal.h +++ b/shared/nm-utils/nm-macros-internal.h @@ -933,12 +933,36 @@ static GParamSpec *obj_properties[_PROPERTY_ENUMS_LAST] = { NULL, } #define NM_GOBJECT_PROPERTIES_DEFINE(obj_type, ...) \ NM_GOBJECT_PROPERTIES_DEFINE_BASE (__VA_ARGS__); \ static inline void \ +_nm_gobject_notify_together_impl (obj_type *obj, guint n, const _PropertyEnums *props) \ +{ \ + const gboolean freeze_thaw = (n > 1); \ + \ + nm_assert (G_IS_OBJECT (obj)); \ + nm_assert (n > 0); \ + \ + if (freeze_thaw) \ + g_object_freeze_notify ((GObject *) obj); \ + while (n-- > 0) { \ + const _PropertyEnums prop = *props++; \ + \ + nm_assert ((gsize) prop < G_N_ELEMENTS (obj_properties)); \ + g_object_notify_by_pspec ((GObject *) obj, obj_properties[prop]); \ + } \ + if (freeze_thaw) \ + g_object_thaw_notify ((GObject *) obj); \ +} \ +\ +static inline void \ _notify (obj_type *obj, _PropertyEnums prop) \ { \ - nm_assert (G_IS_OBJECT (obj)); \ - nm_assert ((gsize) prop < G_N_ELEMENTS (obj_properties)); \ - g_object_notify_by_pspec ((GObject *) obj, obj_properties[prop]); \ -} + _nm_gobject_notify_together_impl (obj, 1, &prop); \ +} \ + +/* invokes _notify() for all arguments (of type _PropertyEnums). Note, that if + * there are more than one prop arguments, this will involve a freeze/thaw + * of GObject property notifications. */ +#define nm_gobject_notify_together(obj, ...) \ + _nm_gobject_notify_together_impl (obj, NM_NARG (__VA_ARGS__), (const _PropertyEnums[]) { __VA_ARGS__ }) /*****************************************************************************/ From 62a4f2652f6a6a4ec4fa42260b4cb40e38752a0f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 14 Jul 2018 17:18:58 +0200 Subject: [PATCH 2/3] core: use nm_gobject_notify_together() in NMIP4Config/NMIP6Config nm_gobject_notify_together() freezes the notifications to emit both notification signals together. That matters for NMDBusObject base class, which hooks into dispatch_properties_changed() to emit a combined "PropertiesChanged" signal. Note, that during calls like nm_ip4_config_replace(), we already froze/thawed the notifications. So, this change adds unnecessary freeze/thaw calls, because signal emition is already frozen. That is a bit ugly, because g_object_freeze_notify() is more heavy than I'd wish it would be. Anyway, for other places, like nm_ip4_config_reset_routes() that is not the case. And correctness trumps performance. Ultimately, the issue there is that we use NMIP4Config / NMIP6Config both to track internal configuration, and to expose it on D-Bus. The majority of created NMIP4Config / NMIP6Config instances won't get exported, and but still pay an unnecessary overhead. The proper solution to minimize the overhead would be, to separate these uses. --- src/nm-ip4-config.c | 8 ++++---- src/nm-ip6-config.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/nm-ip4-config.c b/src/nm-ip4-config.c index 7cb21a7d0..60bac978e 100644 --- a/src/nm-ip4-config.c +++ b/src/nm-ip4-config.c @@ -509,8 +509,8 @@ _notify_addresses (NMIP4Config *self) nm_clear_g_variant (&priv->address_data_variant); nm_clear_g_variant (&priv->addresses_variant); - _notify (self, PROP_ADDRESS_DATA); - _notify (self, PROP_ADDRESSES); + nm_gobject_notify_together (self, PROP_ADDRESS_DATA, + PROP_ADDRESSES); } static void @@ -521,8 +521,8 @@ _notify_routes (NMIP4Config *self) nm_assert (priv->best_default_route == _nm_ip4_config_best_default_route_find (self)); nm_clear_g_variant (&priv->route_data_variant); nm_clear_g_variant (&priv->routes_variant); - _notify (self, PROP_ROUTE_DATA); - _notify (self, PROP_ROUTES); + nm_gobject_notify_together (self, PROP_ROUTE_DATA, + PROP_ROUTES); } /*****************************************************************************/ diff --git a/src/nm-ip6-config.c b/src/nm-ip6-config.c index f0769c152..b166e98f7 100644 --- a/src/nm-ip6-config.c +++ b/src/nm-ip6-config.c @@ -208,8 +208,8 @@ _notify_addresses (NMIP6Config *self) nm_clear_g_variant (&priv->address_data_variant); nm_clear_g_variant (&priv->addresses_variant); - _notify (self, PROP_ADDRESS_DATA); - _notify (self, PROP_ADDRESSES); + nm_gobject_notify_together (self, PROP_ADDRESS_DATA, + PROP_ADDRESSES); } static void @@ -220,8 +220,8 @@ _notify_routes (NMIP6Config *self) nm_assert (priv->best_default_route == _nm_ip6_config_best_default_route_find (self)); nm_clear_g_variant (&priv->route_data_variant); nm_clear_g_variant (&priv->routes_variant); - _notify (self, PROP_ROUTE_DATA); - _notify (self, PROP_ROUTES); + nm_gobject_notify_together (self, PROP_ROUTE_DATA, + PROP_ROUTES); } /*****************************************************************************/ From 4eeb4b1bdd2235eb1895fd9314e21d75427f3552 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 13 Jul 2018 20:15:33 +0200 Subject: [PATCH 3/3] all: avoid byte ordering issue for IP4Config's Nameservers/WinsServers on D-Bus Some properties in NetworkManager's D-Bus API are IPv4 addresses in network byte order (big endian). That is problematic. It is no problem, when the NetworkManager client runs on the same host. That is the case with libnm, which does not support to be used remotely for the time being. It is a problem for an application that wants to access the D-Bus interface of NetworkManager remotely. Possibly, such an application would be implemented in two layers: - one layer merely remotes D-Bus, without specific knowledge of NetworkManager's API. - a higher layer which accesses the remote D-Bus interface of NetworkManager. Preferably it does so in an agnostic way, regardless of whether it runs locally or remotely. When using a D-Bus library, all accesses to 32 bit integers are in native endianness (regardless of how the integer is actually encoded on the lower layers). Likewise, D-Bus does not support annotating integer types in non-native endianness. There is no way to annotate an integer type "u" to be anything but native order. That means, when remoting D-Bus at some point the endianness must be corrected. But by looking at the D-Bus introspection alone, it is not possible to know which property need correction and which don't. One would need to understand the meaning of the properties. That makes it problematic, because the higher layer of the application, which knows that the "Nameservers" property is supposed to be in network order, might not easily know, whether it must correct for endianness. Deprecate IP4Config properties that are only accessible with a particular endianness, and add new properties that expose the same data in an agnostic way. Note that I added "WinsServerData" to be a plain "as", while "NameserverData" is of type "aa{sv}". There is no particularly strong reason for these choices, except that I could imagine that it could be useful to expose additional information in the future about nameservers (e.g. are they received via DHCP or manual configuration?). On the other hand, WINS information likely won't get extended in the future. Also note, libnm was not modified to use the new D-Bus fields. The endianness issue is no problem for libnm, so there is little reason to change it (at this point). https://bugzilla.redhat.com/show_bug.cgi?id=1153559 https://bugzilla.redhat.com/show_bug.cgi?id=1584584 --- ...g.freedesktop.NetworkManager.IP4Config.xml | 23 ++++- libnm/nm-ip-config.c | 2 + src/nm-ip4-config.c | 92 +++++++++++++++---- src/nm-ip4-config.h | 6 +- 4 files changed, 101 insertions(+), 22 deletions(-) diff --git a/introspection/org.freedesktop.NetworkManager.IP4Config.xml b/introspection/org.freedesktop.NetworkManager.IP4Config.xml index 0b04af94e..e4388fd4f 100644 --- a/introspection/org.freedesktop.NetworkManager.IP4Config.xml +++ b/introspection/org.freedesktop.NetworkManager.IP4Config.xml @@ -55,10 +55,20 @@ + + + + + +