Commit d8a31794 authored by Thomas Haller's avatar Thomas Haller

connectivity: rework async connectivity check requests

An asynchronous request should either be cancellable or not keep
the target object alive. Preferably both.

Otherwise, it is impossible to do a controlled shutdown when terminating
NetworkManager. Currently, when NetworkManager is about to terminate,
it just quits the mainloop and essentially leaks everything. That is a
bug. If we ever want to fix that, every asynchronous request must be
cancellable in a controlled way (or it must not prevent objects from
getting disposed, where disposing the object automatically cancels the
callback).

Rework the asynchronous request for connectivity check to

- return a handle that can be used to cancel the operation.
  Cancelling is optional. The caller may choose to ignore the handle
  because the asynchronous operation does not keep the target object
  alive. That means, it is still possible to shutdown, by everybody
  giving up their reference to the target object. In which case the
  callback will be invoked during dispose() of the target object.

- also, the callback will always be invoked exactly once, and never
  synchronously from within the asynchronous start call. But during
  cancel(), the callback is invoked synchronously from within cancel().
  Note that it's only allowed to cancel an action at most once, and
  never after the callback is invoked (also not from within the callback
  itself).

- also, NMConnectivity already supports a fake handler, in case
  connectivity check is disabled via configuration. Hence, reuse
  the same code paths also when compiling without --enable-concheck.
  That means, instead of having #if WITH_CONCHECK at various callers,
  move them into NMConnectivity. The downside is, that if you build
  without concheck, there is a small overhead compared to before. The
  upside is, we reuse the same code paths when compiling with or without
  concheck.

- also, the patch synchronizes the connecitivty states. For example,
  previously `nmcli networking connectivity check` would schedule
  requests in parallel, and return the accumulated result of the individual
  requests.
  However, the global connectivity state of the manager might have have
  been the same as the answer to the explicit connecitivity check,
  because while the answer for the manual check is waiting for all
  pending checks to complete, the global connectivity state could
  already change. That is just wrong. There are not multiple global
  connectivity states at the same time, there is just one. A manual
  connectivity check should have the meaning of ensure that the global
  state is up to date, but it still should return the global
  connectivity state -- not the answers for several connectivity checks
  issued in parallel.
  This is related to commit b799de28
  (libnm: update property in the manager after connectivity check),
  which tries to address a similar problem client side.
  Similarly, each device has a connectivity state. While there might
  be several connectivity checks per device pending, whenever a check
  completes, it can update the per-device state (and return that device
  state as result), but the immediate answer of the individual check
  might not matter. This is especially the case, when a later request
  returns earlier and obsoletes all earlier requests. In that case,
  earlier requests return with the result of the currend devices
  connectivity state.

This patch cleans up the internal API and gives a better defined behavior
to the user (thus, the simple API which simplifies implementation for the
caller). However, the implementation of getting this API right and properly
handle cancel and destruction of the target object is more complicated and
complex. But this but is not just for the sake of a nicer API. This fixes
actual issues explained above.

Also, get rid of GAsyncResult to track information about the pending request.
Instead, allocate our own handle structure, which ends up to be nicer
because it's strongly typed and has exactly the properties that are
useful to track the request. Also, it gets rid of the awkward
_finish() API by passing the relevant arguments to the callback
directly.
parent 4417b8bf
This diff is collapsed.
......@@ -775,12 +775,20 @@ gboolean nm_device_hw_addr_get_cloned (NMDevice *self,
gboolean *preserve,
GError **error);
typedef struct _NMDeviceConnectivityHandle NMDeviceConnectivityHandle;
typedef void (*NMDeviceConnectivityCallback) (NMDevice *self,
NMDeviceConnectivityHandle *handle,
NMConnectivityState state,
GError *error,
gpointer user_data);
void nm_device_check_connectivity (NMDevice *self,
NMDeviceConnectivityCallback callback,
gpointer user_data);
NMDeviceConnectivityHandle *nm_device_check_connectivity (NMDevice *self,
NMDeviceConnectivityCallback callback,
gpointer user_data);
void nm_device_check_connectivity_cancel (NMDeviceConnectivityHandle *handle);
NMConnectivityState nm_device_get_connectivity_state (NMDevice *self);
typedef struct _NMBtVTableNetworkServer NMBtVTableNetworkServer;
......
......@@ -402,9 +402,7 @@ main (int argc, char *argv[])
manager))
goto done;
#if WITH_CONCHECK
NM_UTILS_KEEP_ALIVE (manager, nm_connectivity_get (), "NMManager-depends-on-NMConnectivity");
#endif
nm_dispatcher_init ();
......
This diff is collapsed.
......@@ -24,6 +24,9 @@
#include "nm-dbus-interface.h"
#define NM_CONNECTIVITY_ERROR ((NMConnectivityState) -1)
#define NM_CONNECTIVITY_FAKE ((NMConnectivityState) -2)
#define NM_TYPE_CONNECTIVITY (nm_connectivity_get_type ())
#define NM_CONNECTIVITY(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), NM_TYPE_CONNECTIVITY, NMConnectivity))
#define NM_CONNECTIVITY_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), NM_TYPE_CONNECTIVITY, NMConnectivityClass))
......@@ -41,13 +44,21 @@ NMConnectivity *nm_connectivity_get (void);
const char *nm_connectivity_state_to_string (NMConnectivityState state);
void nm_connectivity_check_async (NMConnectivity *self,
const char *iface,
GAsyncReadyCallback callback,
gpointer user_data);
NMConnectivityState nm_connectivity_check_finish (NMConnectivity *self,
GAsyncResult *result,
GError **error);
gboolean nm_connectivity_check_enabled (NMConnectivity *self);
typedef struct _NMConnectivityCheckHandle NMConnectivityCheckHandle;
typedef void (*NMConnectivityCheckCallback) (NMConnectivity *self,
NMConnectivityCheckHandle *handle,
NMConnectivityState state,
GError *error,
gpointer user_data);
NMConnectivityCheckHandle *nm_connectivity_check_start (NMConnectivity *self,
const char *iface,
NMConnectivityCheckCallback callback,
gpointer user_data);
void nm_connectivity_check_cancel (NMConnectivityCheckHandle *handle);
#endif /* __NETWORKMANAGER_CONNECTIVITY_H__ */
......@@ -2450,7 +2450,6 @@ device_realized (NMDevice *device,
_emit_device_added_removed (self, device, nm_device_is_real (device));
}
#if WITH_CONCHECK
static void
device_connectivity_changed (NMDevice *device,
GParamSpec *pspec,
......@@ -2478,7 +2477,6 @@ device_connectivity_changed (NMDevice *device,
nm_dispatcher_call_connectivity (priv->connectivity_state, NULL, NULL, NULL);
}
}
#endif
static void
_device_realize_finish (NMManager *self,
......@@ -2588,11 +2586,9 @@ add_device (NMManager *self, NMDevice *device, GError **error)
G_CALLBACK (device_realized),
self);
#if WITH_CONCHECK
g_signal_connect (device, "notify::" NM_DEVICE_CONNECTIVITY,
G_CALLBACK (device_connectivity_changed),
self);
#endif
if (priv->startup) {
g_signal_connect (device, "notify::" NM_DEVICE_HAS_PENDING_ACTION,
......@@ -5441,34 +5437,54 @@ impl_manager_get_logging (NMDBusObject *obj,
}
typedef struct {
guint remaining;
NMManager *self;
GDBusMethodInvocation *context;
NMConnectivityState state;
guint remaining;
} ConnectivityCheckData;
static void
device_connectivity_done (NMDevice *device, NMConnectivityState state, gpointer user_data)
device_connectivity_done (NMDevice *device,
NMDeviceConnectivityHandle *handle,
NMConnectivityState state,
GError *error,
gpointer user_data)
{
ConnectivityCheckData *data = user_data;
NMManager *self;
NMManagerPrivate *priv;
data->remaining--;
nm_assert (data);
nm_assert (data->remaining > 0);
nm_assert (NM_IS_MANAGER (data->self));
/* We check if the state is already FULL so that we can provide the
* response without waiting for slower devices that are not going to
* affect the overall state anyway. */
data->remaining--;
if (data->state != NM_CONNECTIVITY_FULL) {
if (state > data->state)
data->state = state;
self = data->self;
priv = NM_MANAGER_GET_PRIVATE (self);
if (data->state == NM_CONNECTIVITY_FULL || !data->remaining) {
g_dbus_method_invocation_return_value (data->context,
g_variant_new ("(u)", data->state));
}
if ( data->context
&& ( data->remaining == 0
|| ( state == NM_CONNECTIVITY_FULL
&& priv->connectivity_state == NM_CONNECTIVITY_FULL))) {
/* despite having a @handle and @state returned by the requests, we always
* return the current connectivity_state. That is, because the connectivity_state
* and the answer to the connectivity check shall agree.
*
* However, if one of the requests (early) returns full connectivity and agrees with
* the accumulated connectivity state, we no longer have to wait. The result is set.
*
* This also works well, because NMDevice first emits change signals to it's own
* connectivity state, which is then taken into account for the accumulated global
* state. All this happens, before the callback is invoked. */
g_dbus_method_invocation_return_value (g_steal_pointer (&data->context),
g_variant_new ("(u)",
(guint) priv->connectivity_state));
}
if (!data->remaining)
if (data->remaining == 0) {
g_object_unref (self);
g_slice_free (ConnectivityCheckData, data);
}
}
static void
......@@ -5482,6 +5498,7 @@ check_connectivity_auth_done_cb (NMAuthChain *chain,
GError *error = NULL;
NMAuthCallResult result;
ConnectivityCheckData *data;
NMDevice *device;
priv->auth_chains = g_slist_remove (priv->auth_chains, chain);
......@@ -5497,23 +5514,37 @@ check_connectivity_auth_done_cb (NMAuthChain *chain,
error = g_error_new_literal (NM_MANAGER_ERROR,
NM_MANAGER_ERROR_PERMISSION_DENIED,
"Not authorized to recheck connectivity");
} else {
NMDevice *device;
}
/* it's allowed */
data = g_slice_new0 (ConnectivityCheckData);
data->context = context;
if (error) {
g_dbus_method_invocation_take_error (context, error);
goto out;
}
c_list_for_each_entry (device, &priv->devices_lst_head, devices_lst) {
data->remaining++;
nm_device_check_connectivity (device,
device_connectivity_done,
data);
}
data = g_slice_new (ConnectivityCheckData);
data->self = g_object_ref (self);
data->context = context;
data->remaining = 0;
c_list_for_each_entry (device, &priv->devices_lst_head, devices_lst) {
data->remaining++;
nm_device_check_connectivity (device,
device_connectivity_done,
data);
}
if (error)
g_dbus_method_invocation_take_error (context, error);
if (data->remaining == 0) {
/* call the handler at least once. */
data->remaining = 1;
device_connectivity_done (NULL,
NULL,
NM_CONNECTIVITY_UNKNOWN,
NULL,
data);
/* @data got destroyed. */
}
out:
nm_auth_chain_unref (chain);
}
......@@ -6559,7 +6590,6 @@ get_property (GObject *object, guint prop_id,
const char *path;
NMActiveConnection *ac;
GPtrArray *ptrarr;
gboolean vbool;
switch (prop_id) {
case PROP_VERSION:
......@@ -6616,12 +6646,8 @@ get_property (GObject *object, guint prop_id,
g_value_set_boolean (value, nm_config_data_get_connectivity_uri (config_data) != NULL);
break;
case PROP_CONNECTIVITY_CHECK_ENABLED:
#if WITH_CONCHECK
vbool = nm_connectivity_check_enabled (nm_connectivity_get ());
#else
vbool = FALSE;
#endif
g_value_set_boolean (value, vbool);
g_value_set_boolean (value,
nm_connectivity_check_enabled (nm_connectivity_get ()));
break;
case PROP_PRIMARY_CONNECTION:
nm_dbus_utils_g_value_set_object_path (value, priv->primary_connection);
......
......@@ -318,10 +318,10 @@ test_config_global_dns (void)
g_object_unref (config);
}
#if WITH_CONCHECK
static void
test_config_connectivity_check (void)
{
#if WITH_CONCHECK
const char *CONFIG_INTERN = BUILDDIR"/test-connectivity-check-intern.conf";
NMConfig *config;
NMConnectivity *connectivity;
......@@ -351,8 +351,10 @@ test_config_connectivity_check (void)
g_object_unref (config);
g_assert (remove (CONFIG_INTERN) == 0);
}
#else
g_test_skip ("concheck disabled");
#endif
}
static void
test_config_no_auto_default (void)
......@@ -1048,9 +1050,7 @@ main (int argc, char **argv)
g_test_add_func ("/config/set-values", test_config_set_values);
g_test_add_func ("/config/global-dns", test_config_global_dns);
#if WITH_CONCHECK
g_test_add_func ("/config/connectivity-check", test_config_connectivity_check);
#endif
g_test_add_func ("/config/signal", test_config_signal);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment