summaryrefslogtreecommitdiff
path: root/libnm/nm-vpn-connection.c
diff options
context:
space:
mode:
authorThomas Haller <thaller@redhat.com>2019-10-30 11:42:58 +0100
committerThomas Haller <thaller@redhat.com>2019-11-11 07:55:33 +0100
commit36d8637741540db91e4544ede7333c7ba0da54c5 (patch)
tree45af734ddbf73b86f106422da1872abf3495a49e /libnm/nm-vpn-connection.c
parent031ee2e8df065d440762ed89b375cd34a766e22f (diff)
downloadNetworkManager-th/libnm-no-dbus-codegen-4.tar.gz
libnm: refactor caching of D-Bus objects in NMClientth/libnm-no-dbus-codegen-4
No longer use GDBusObjectMangaerClient and gdbus-codegen generated classes for the NMClient cache. Instead, use GDBusConnection directly and use a custom implementation (NMLDBusObject) for caching the ObjectManager data. CHANGES ------- - This is a complete rework. I think the previous implementation was difficult to understand. There were unfixed bugs and nobody understood the code well enough to fix them. Maybe somebody out there understood the code, but I certainly did not. At least nobody provided patches to fix those issues. I do believe that this implementation is more straightforward and easier to understand. It removes a lot of layers of code. Whether this claim of simplicity is true, each reader must decide for himself/herself. Note that it is still fairly complex. - There was a lingering performance issue with large number of D-Bus objects. The patch tries hard that the implementation scales well. Of course, when we cache N objects that have N-to-M references to other, we still are fundamentally O(N*M) for runtime and memory consumption (with M being the number of references between objects). But each part should behave efficiently and well. - Play well with GMainContext. libnm code (NMClient) is generally not thread safe. However, it should work to use multiple instances in parallel, as long as each access to a NMClient is through the caller's GMainContext. This follows glib's style and effectively allows to use NMClient in a multi threaded scenario. This implies to stick to a main context upon construction and ensure that callbacks are only invoked when iterating that context. Also, NMClient itself shall never iterate the callers context. This also means, libnm must never use g_idle_add() or g_timeout_add(), as those enqueue sources in the g_main_context_default() context. - Get ordering of messages right. All events are consistently enqueued in a GMainContext and processed strictly in order. For example, previously nm-object.c tried to combine signals and emit them on an idle handler. That is wrong, signals must be emitted in the right order and when they happen. Note that when using GInitable's synchronous initialization to initialize the NMClient instance, then we still operate (internally) fully asynchronous. To get this right NMClient has an internal main context. - NMClient takes over most of the functionality. When using D-Bus' ObjectManager interface, one needs to handle basically the entire state of the D-Bus interface. That cannot be separated well into distinct parts, and even if you try, you just end up having closely related code in different source files. Spreading related code does not make it easier to understand, on the contrary. That means, NMClient is inherently complex as it contains most of the logic. I think that is not avoidable, but it's not as bad as it sounds (IMO). - NMClient processes D-Bus messages and state changes in separate steps. First NMClient unpacks the message (e.g. _dbus_handle_properties_changed()) and keep track of the changed data. Then we update the GObject instances (_dbus_handle_objects_changed()) without emitting any signals yet. Finally, we emit all signals and notifications that were collected (_dbus_handle_changes_commit()). Note that for example during the initial GetManagedObjects() reply, NMClient receive a large amount of state at once. But we first apply all the state to our GObject instances before starting to emit any signals. The result is that signals are always emitted in a moment when the cache is consistent. The unavoidable downside is that when you receive a property changed signal, possibly many other properties changed at the same time. - NMDeviceWifi no longer modifies the content of the cache from client side during poke_wireless_devices_with_rf_status(). The content of the cache should be determined by D-Bus alone and follow what NetworkManager service exposes. Local modifications should be avoided. - This aims to bring no API/ABI change, though it does of course bring various subtle changes in behavior. Those should be all for the better, but the goal is not to break any existing clients. This does change internal (albeit externally visible) API, like dropping NM_OBJECT_DBUS_OBJECT_MANAGER property and NMObject no longer implementing GInitableIface and GAsyncInitableIface. - Some uses of gdbus-codegen classes remain in NMVpnPluginOld, NMVpnServicePlugin and NMSecretAgentOld. These are independent of NMClient/NMObject and should be reworked separately. - While we no longer use generated classes from gdbus-codegen, we don't need more glue code than before. Also before there was NMPropertiesInfo and a large amount of code that propagated properties from NMDBus* to NMObject. That got completely reworked, but did not fundamentally change. You still need about the same effort to create the NMLDBusMetaIface and not using generated bindings did not make anything worse. - NMLDBusMetaIface and other meta data is static and immutable. This avoids copying them around. Also, macros like NML_DBUS_META_PROPERTY_INIT_U() have compile time checks to ensure the property types matches. It's pretty hard to misuse them because it won't compile. - The meta data now explicitly encodes the expected D-Bus types and makes sure never to accept wrong data. That would only matter when the server (accidentally or intentionally) exposes unexpected types on D-Bus. I don't think that was previously ensured in all cases. For example, demarshal_generic() only cared about the GObject property type, it didn't know the expected D-Bus type. - Previously GDBusObjectManager would sometimes emit warnings (g_log()). Those probably indicated real bugs. In any case, it prevented us from running CI with G_DEBUG=fatal-warnings, because there would be just too many unrelated crashes. Now we log debug messages that can be enabled with "LIBNM_CLIENT_DEBUG=trace". Some of these messages can also be turned into g_warning()/g_critical() by setting LIBNM_CLIENT_DEBUG=warning,error. Together with G_DEBUG=fatal-warnings, this makes them into assertions. Note that such "assertion failures" might also happen because of a server bug (or change). Thus these are not common assertions that indicate a bug in libnm and are thus not armed unless explicitly requested. In our CI we should now always run with LIBNM_CLIENT_DEBUG=warning,error and G_DEBUG=fatal-warnings and to catch bugs. - Note that this changes the order in which we emit "notify:devices" and "device-added" signals. I think it makes the most sense to emit first "device-removed", then "notify:devices", and finally "device-added" signals. This changes behavior for commit 52ae28f6e5bf ('libnm: queue added/removed signals and suppress uninitialized notifications'), but I don't think that users should actually rely on the order. Still, the new order makes the most sense to me. - In NetworkManager, profiles can be invisible to the user by setting "connection.permissions". Such profiles would be hidden by NMClient's nm_client_get_connections() and their "connection-added"/"connection-removed" signals. Note that NMActiveConnection's nm_active_connection_get_connection() and NMDevice's nm_device_get_available_connections() still exposes such hidden NMRemoteConnection instances. This behavior was preserved. NUMBERS ------- I compared 3 versions of libnm. [1] CCCCCCC, currently latest on nm-1-20 branch [2] CCCCCCC, current master, before this patch [3] this patch All tests were done on Fedora 31, x86_64, gcc 9.2.1-1.fc31. The libraries were build with $ ./contrib/fedora/rpm/build_clean.sh -g -w test -W debug Note that RPM build already stripped the library. A) file size of libnm.so.0.1.0 [1] 2769712 [2] [3] B) `size libnm.so.0.1.0`: text data bss dec hex filename [1] 1534988 73428 13232 1621648 18be90 /usr/lib64/libnm.so.0.1.0 [2] [3] WIP
Diffstat (limited to 'libnm/nm-vpn-connection.c')
-rw-r--r--libnm/nm-vpn-connection.c100
1 files changed, 57 insertions, 43 deletions
diff --git a/libnm/nm-vpn-connection.c b/libnm/nm-vpn-connection.c
index a0d673e913..26980229f3 100644
--- a/libnm/nm-vpn-connection.c
+++ b/libnm/nm-vpn-connection.c
@@ -14,8 +14,6 @@
#include "nm-active-connection.h"
#include "nm-dbus-helpers.h"
-#include "introspection/org.freedesktop.NetworkManager.VPN.Connection.h"
-
/*****************************************************************************/
NM_GOBJECT_PROPERTIES_DEFINE (NMVpnConnection,
@@ -33,7 +31,8 @@ static guint signals[LAST_SIGNAL] = { 0 };
typedef struct {
char *banner;
- NMVpnConnectionState vpn_state;
+ guint32 vpn_state;
+ guint32 reason;
} NMVpnConnectionPrivate;
struct _NMVpnConnection {
@@ -86,20 +85,48 @@ nm_vpn_connection_get_vpn_state (NMVpnConnection *vpn)
return NM_VPN_CONNECTION_GET_PRIVATE (vpn)->vpn_state;
}
+/*****************************************************************************/
+
static void
-vpn_state_changed_proxy (NMDBusVpnConnection *proxy,
- guint vpn_state,
- guint reason,
- gpointer user_data)
+_notify_event_state_changed (NMClient *client,
+ NMClientNotifyEventWithPtr *notify_event)
+{
+ gs_unref_object NMVpnConnection *self = notify_event->user_data;
+ NMVpnConnectionPrivate *priv = NM_VPN_CONNECTION_GET_PRIVATE (self);
+
+ /* we expose here the value cache in @priv. In practice, this is the same
+ * value as we received from the signal. In the unexpected case where they
+ * differ, the cached value of the current instance would still be more correct. */
+ g_signal_emit (self,
+ signals[VPN_STATE_CHANGED],
+ 0,
+ (guint) priv->vpn_state,
+ (guint) priv->reason);
+}
+
+void
+_nm_vpn_connection_state_changed_commit (NMVpnConnection *self,
+ guint32 state,
+ guint32 reason)
{
- NMVpnConnection *connection = NM_VPN_CONNECTION (user_data);
- NMVpnConnectionPrivate *priv = NM_VPN_CONNECTION_GET_PRIVATE (connection);
+ NMClient *client;
+ NMVpnConnectionPrivate *priv = NM_VPN_CONNECTION_GET_PRIVATE (self);
+
+ client = _nm_object_get_client (self);
- if (priv->vpn_state != vpn_state) {
- priv->vpn_state = vpn_state;
- g_signal_emit (connection, signals[VPN_STATE_CHANGED], 0, vpn_state, reason);
- _notify (connection, PROP_VPN_STATE);
+ if (priv->vpn_state != state) {
+ priv->vpn_state = state;
+ _nm_client_queue_notify_object (client,
+ self,
+ obj_properties[PROP_VPN_STATE]);
}
+
+ priv->reason = reason;
+
+ _nm_client_notify_event_queue_with_ptr (client,
+ NM_CLIENT_NOTIFY_EVENT_PRIO_GPROP + 1,
+ _notify_event_state_changed,
+ g_object_ref (self));
}
/*****************************************************************************/
@@ -107,32 +134,6 @@ vpn_state_changed_proxy (NMDBusVpnConnection *proxy,
static void
nm_vpn_connection_init (NMVpnConnection *connection)
{
- NMVpnConnectionPrivate *priv = NM_VPN_CONNECTION_GET_PRIVATE (connection);
-
- priv->vpn_state = NM_VPN_CONNECTION_STATE_UNKNOWN;
-}
-
-static void
-init_dbus (NMObject *object)
-{
- NMVpnConnectionPrivate *priv = NM_VPN_CONNECTION_GET_PRIVATE (object);
- const NMPropertiesInfo property_info[] = {
- { NM_VPN_CONNECTION_BANNER, &priv->banner },
- { NM_VPN_CONNECTION_VPN_STATE, &priv->vpn_state },
- { NULL },
- };
- GDBusProxy *proxy;
-
- NM_OBJECT_CLASS (nm_vpn_connection_parent_class)->init_dbus (object);
-
- _nm_object_register_properties (object,
- NM_DBUS_INTERFACE_VPN_CONNECTION,
- property_info);
-
- proxy = _nm_object_get_proxy (object, NM_DBUS_INTERFACE_VPN_CONNECTION);
- g_signal_connect_object (proxy, "vpn-state-changed",
- G_CALLBACK (vpn_state_changed_proxy), object, 0);
- g_object_unref (proxy);
}
static void
@@ -166,17 +167,24 @@ get_property (GObject *object,
}
}
+const NMLDBusMetaIface _nml_dbus_meta_iface_nm_vpn_connection = NML_DBUS_META_IFACE_INIT_PROP (
+ NM_DBUS_INTERFACE_VPN_CONNECTION,
+ nm_vpn_connection_get_type,
+ NML_DBUS_META_INTERFACE_PRIO_INSTANTIATE_HIGH,
+ NML_DBUS_META_IFACE_DBUS_PROPERTIES (
+ NML_DBUS_META_PROPERTY_INIT_S ("Banner", PROP_BANNER, NMVpnConnection, _priv.banner ),
+ NML_DBUS_META_PROPERTY_INIT_U ("VpnState", PROP_VPN_STATE, NMVpnConnection, _priv.vpn_state ),
+ ),
+);
+
static void
nm_vpn_connection_class_init (NMVpnConnectionClass *connection_class)
{
GObjectClass *object_class = G_OBJECT_CLASS (connection_class);
- NMObjectClass *nm_object_class = NM_OBJECT_CLASS (connection_class);
object_class->get_property = get_property;
object_class->finalize = finalize;
- nm_object_class->init_dbus = init_dbus;
-
/**
* NMVpnConnection:vpn-state:
*
@@ -200,7 +208,13 @@ nm_vpn_connection_class_init (NMVpnConnectionClass *connection_class)
G_PARAM_READABLE |
G_PARAM_STATIC_STRINGS);
- g_object_class_install_properties (object_class, _PROPERTY_ENUMS_LAST, obj_properties);
+ _nml_dbus_meta_class_init_with_properties (object_class, &_nml_dbus_meta_iface_nm_vpn_connection);
+
+ /* TODO: the state reason should also be exposed as a property in libnm's NMVpnConnection,
+ * like done for NMDevice's state reason. */
+
+ /* TODO: the D-Bus API should also expose the state-reason as a property instead of
+ * a "VpnStateChanged" signal. Like done for Device's "StateReason". */
G_GNUC_BEGIN_IGNORE_DEPRECATIONS
signals[VPN_STATE_CHANGED] =