fdo-mirrors/NetworkManager
1
0
Fork 0
mirror of https://gitlab.freedesktop.org/NetworkManager/NetworkManager.git synced 2026-05-09 08:18:03 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions

dhcp: replace NMDhcpClient's signals with "notify" and one notify data argument

Browse source 
NMDhcpClient communicates events via GObject signals. GObject signals in
principle could have several subscribers. In practice, a NMDhcpClient
instance has only one subscriber, because it was constructed with
certain parameters, so it's unlikely to be shared.

That one subscriber, always needs to subscribe to all signals
("state-changed" and "prefix-delegated"), Unless the subscriber only
creates a IPv4 client. In which case they won't subscribe to
"prefix-delegated", but that signal is also not invoked for IPv4
clients.

Combine the signals in one, and pass all parameters via a new
NMDhcpClientNotfiyData payload. I feel this is nicer, to pack all
parameters together. I find this more type-aware, where we can
switch (in the callback) based on a notify-type enum, instead
of subscribing multiple signal handlers.

With l3cfg work, DHCP handling will be refactored, where this model of
having one "generic" notify signal makes more sense than here. For the
moment, it is arguably pretty much the same. Also, because NMDhcpClient
subscribes two different handlers for IPv4 and IPv6. In the future,
there will be only one notify handler, and that can cover IPv4 and IPv6
and both "state-changed" and "prefix-delegated" (and other notification
types).
This commit is contained in:
Thomas Haller 2021-08-06 08:07:35 +02:00
parent 4d0e295317
commit dbdd8303fc
No known key found for this signature in database
GPG key ID: 29C2366E4DFC5728
5 changed files with 135 additions and 100 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

97
src/core/devices/nm-device.c
View file

@ -185,7 +185,7 @@ typedef struct {
typedef struct { typedef struct {
NMDhcpClient *client; NMDhcpClient *client;
NMDhcpConfig *config; NMDhcpConfig *config;
gulong state_sigid; gulong notify_sigid;
guint grace_id; guint grace_id;
bool grace_pending : 1; bool grace_pending : 1;
bool was_active : 1; bool was_active : 1;
@ -579,7 +579,6 @@ typedef struct _NMDevicePrivate {
AppliedConfig ip6_config; AppliedConfig ip6_config;
/* Event ID of the current IP6 config from DHCP */ /* Event ID of the current IP6 config from DHCP */
char * event_id; char * event_id;
gulong prefix_sigid;
NMNDiscDHCPLevel mode; NMNDiscDHCPLevel mode;
guint needed_prefixes; guint needed_prefixes;
} dhcp6; } dhcp6;
@ -9035,7 +9034,7 @@ dhcp4_cleanup(NMDevice *self, CleanupType cleanup_type, gboolean release)
if (priv->dhcp_data_4.client) { if (priv->dhcp_data_4.client) {
/* Stop any ongoing DHCP transaction on this device */ /* Stop any ongoing DHCP transaction on this device */
nm_clear_g_signal_handler(priv->dhcp_data_4.client, &priv->dhcp_data_4.state_sigid); nm_clear_g_signal_handler(priv->dhcp_data_4.client, &priv->dhcp_data_4.notify_sigid);
if (cleanup_type == CLEANUP_TYPE_DECONFIGURE || cleanup_type == CLEANUP_TYPE_REMOVED) if (cleanup_type == CLEANUP_TYPE_DECONFIGURE || cleanup_type == CLEANUP_TYPE_REMOVED)
nm_dhcp_client_stop(priv->dhcp_data_4.client, release); nm_dhcp_client_stop(priv->dhcp_data_4.client, release);
@ -9436,21 +9435,27 @@ dhcp4_dad_cb(NMDevice *self, NMIP4Config **configs, gboolean success)
} }
static void static void
dhcp4_state_changed(NMDhcpClient *client, dhcp4_notify(NMDhcpClient *client, const NMDhcpClientNotifyData *notify_data, NMDevice *self)
NMDhcpState state,
NMIP4Config * ip4_config,
GHashTable * options,
gpointer user_data)
{ {
NMDevice * self = NM_DEVICE(user_data);
NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self);
NMIP4Config * manual, **configs; NMIP4Config * manual;
NMIP4Config ** configs;
NMConnection * connection; NMConnection * connection;
NMDhcpState state;
NMIP4Config * ip4_config;
GHashTable * options;
g_return_if_fail(nm_dhcp_client_get_addr_family(client) == AF_INET); nm_assert(nm_dhcp_client_get_addr_family(client) == AF_INET);
g_return_if_fail(!ip4_config || NM_IS_IP4_CONFIG(ip4_config)); nm_assert(notify_data);
nm_assert(notify_data->notify_type == NM_DHCP_CLIENT_NOTIFY_TYPE_STATE_CHANGED);
_LOGD(LOGD_DHCP4, "new DHCPv4 client state %d", state); state = notify_data->state_changed.dhcp_state;
ip4_config = NM_IP4_CONFIG(notify_data->state_changed.ip_config);
options = notify_data->state_changed.options;
nm_assert(!ip4_config || NM_IS_IP4_CONFIG(ip4_config));
_LOGD(LOGD_DHCP4, "new DHCPv4 client state %d", (int) state);
switch (state) { switch (state) {
case NM_DHCP_STATE_BOUND: case NM_DHCP_STATE_BOUND:
@ -9601,10 +9606,10 @@ dhcp4_start(NMDevice *self)
return NM_ACT_STAGE_RETURN_FAILURE; return NM_ACT_STAGE_RETURN_FAILURE;
} }
priv->dhcp_data_4.state_sigid = g_signal_connect(priv->dhcp_data_4.client, priv->dhcp_data_4.notify_sigid = g_signal_connect(priv->dhcp_data_4.client,
NM_DHCP_CLIENT_SIGNAL_STATE_CHANGED, NM_DHCP_CLIENT_NOTIFY,
G_CALLBACK(dhcp4_state_changed), G_CALLBACK(dhcp4_notify),
self); self);
if (nm_device_sys_iface_state_is_external_or_assume(self)) if (nm_device_sys_iface_state_is_external_or_assume(self))
priv->dhcp_data_4.was_active = TRUE; priv->dhcp_data_4.was_active = TRUE;
@ -9775,8 +9780,7 @@ dhcp6_cleanup(NMDevice *self, CleanupType cleanup_type, gboolean release)
priv->dhcp_data_6.grace_pending = FALSE; priv->dhcp_data_6.grace_pending = FALSE;
if (priv->dhcp_data_6.client) { if (priv->dhcp_data_6.client) {
nm_clear_g_signal_handler(priv->dhcp_data_6.client, &priv->dhcp_data_6.state_sigid); nm_clear_g_signal_handler(priv->dhcp_data_6.client, &priv->dhcp_data_6.notify_sigid);
nm_clear_g_signal_handler(priv->dhcp_data_6.client, &priv->dhcp6.prefix_sigid);
if (cleanup_type == CLEANUP_TYPE_DECONFIGURE || cleanup_type == CLEANUP_TYPE_REMOVED) if (cleanup_type == CLEANUP_TYPE_DECONFIGURE || cleanup_type == CLEANUP_TYPE_REMOVED)
nm_dhcp_client_stop(priv->dhcp_data_6.client, release); nm_dhcp_client_stop(priv->dhcp_data_6.client, release);
@ -9876,20 +9880,34 @@ clear_config:
} }
static void static void
dhcp6_state_changed(NMDhcpClient *client, dhcp6_notify(NMDhcpClient *client, const NMDhcpClientNotifyData *notify_data, NMDevice *self)
NMDhcpState state,
NMIP6Config * ip6_config,
GHashTable * options,
gpointer user_data)
{ {
NMDevice * self = NM_DEVICE(user_data);
NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self);
gs_free char * event_id = NULL; gs_free char * event_id = NULL;
NMDhcpState state;
NMIP6Config * ip6_config;
GHashTable * options;
g_return_if_fail(nm_dhcp_client_get_addr_family(client) == AF_INET6); nm_assert(nm_dhcp_client_get_addr_family(client) == AF_INET6);
g_return_if_fail(!ip6_config || NM_IS_IP6_CONFIG(ip6_config)); nm_assert(notify_data);
_LOGD(LOGD_DHCP6, "new DHCPv6 client state %d", state); if (notify_data->notify_type == NM_DHCP_CLIENT_NOTIFY_TYPE_PREFIX_DELEGATED) {
/* Just re-emit. The device just contributes the prefix to the
* pool in NMPolicy, which decides about subnet allocation
* on the shared devices. */
g_signal_emit(self, signals[IP6_PREFIX_DELEGATED], 0, notify_data->prefix_delegated.prefix);
return;
}
nm_assert(notify_data->notify_type == NM_DHCP_CLIENT_NOTIFY_TYPE_STATE_CHANGED);
state = notify_data->state_changed.dhcp_state;
ip6_config = NM_IP6_CONFIG(notify_data->state_changed.ip_config);
options = notify_data->state_changed.options;
nm_assert(!ip6_config || NM_IS_IP6_CONFIG(ip6_config));
_LOGD(LOGD_DHCP6, "new DHCPv6 client state %d", (int) state);
switch (state) { switch (state) {
case NM_DHCP_STATE_BOUND: case NM_DHCP_STATE_BOUND:
@ -9969,17 +9987,6 @@ dhcp6_state_changed(NMDhcpClient *client,
} }
} }
static void
dhcp6_prefix_delegated(NMDhcpClient *client, const NMPlatformIP6Address *prefix, gpointer user_data)
{
NMDevice *self = NM_DEVICE(user_data);
/* Just re-emit. The device just contributes the prefix to the
* pool in NMPolicy, which decides about subnet allocation
* on the shared devices. */
g_signal_emit(self, signals[IP6_PREFIX_DELEGATED], 0, prefix);
}
/*****************************************************************************/ /*****************************************************************************/
static gboolean static gboolean
@ -10061,14 +10068,10 @@ dhcp6_start_with_link_ready(NMDevice *self, NMConnection *connection)
return FALSE; return FALSE;
} }
priv->dhcp_data_6.state_sigid = g_signal_connect(priv->dhcp_data_6.client, priv->dhcp_data_6.notify_sigid = g_signal_connect(priv->dhcp_data_6.client,
NM_DHCP_CLIENT_SIGNAL_STATE_CHANGED, NM_DHCP_CLIENT_NOTIFY,
G_CALLBACK(dhcp6_state_changed), G_CALLBACK(dhcp6_notify),
self); self);
priv->dhcp6.prefix_sigid = g_signal_connect(priv->dhcp_data_6.client,
NM_DHCP_CLIENT_SIGNAL_PREFIX_DELEGATED,
G_CALLBACK(dhcp6_prefix_delegated),
self);
if (nm_device_sys_iface_state_is_external_or_assume(self)) if (nm_device_sys_iface_state_is_external_or_assume(self))
priv->dhcp_data_6.was_active = TRUE; priv->dhcp_data_6.was_active = TRUE;

68
src/core/dhcp/nm-dhcp-client.c
View file

@ -28,7 +28,7 @@
/*****************************************************************************/ /*****************************************************************************/
enum { SIGNAL_STATE_CHANGED, SIGNAL_PREFIX_DELEGATED, LAST_SIGNAL }; enum { SIGNAL_NOTIFY, LAST_SIGNAL };
static guint signals[LAST_SIGNAL] = {0}; static guint signals[LAST_SIGNAL] = {0};
@ -91,6 +91,33 @@ G_STATIC_ASSERT(!(((pid_t) -1) > 0));
/*****************************************************************************/ /*****************************************************************************/
static void
_emit_notify(NMDhcpClient *self, const NMDhcpClientNotifyData *notify_data)
{
g_signal_emit(G_OBJECT(self), signals[SIGNAL_NOTIFY], 0, notify_data);
}
static void
_emit_notify_state_changed(NMDhcpClient *self,
NMDhcpState dhcp_state,
NMIPConfig * ip_config,
GHashTable * options)
{
const NMDhcpClientNotifyData notify_data = {
.notify_type = NM_DHCP_CLIENT_NOTIFY_TYPE_STATE_CHANGED,
.state_changed =
{
.dhcp_state = dhcp_state,
.ip_config = ip_config,
.options = options,
},
};
_emit_notify(self, &notify_data);
}
/*****************************************************************************/
pid_t pid_t
nm_dhcp_client_get_pid(NMDhcpClient *self) nm_dhcp_client_get_pid(NMDhcpClient *self)
{ {
@ -496,12 +523,8 @@ nm_dhcp_client_set_state(NMDhcpClient *self,
} }
priv->state = new_state; priv->state = new_state;
g_signal_emit(G_OBJECT(self),
signals[SIGNAL_STATE_CHANGED], _emit_notify_state_changed(self, new_state, ip_config, options);
0,
(guint) new_state,
ip_config,
options);
} }
static gboolean static gboolean
@ -873,7 +896,15 @@ maybe_add_option(NMDhcpClient *self, GHashTable *hash, const char *key, GVariant
void void
nm_dhcp_client_emit_ipv6_prefix_delegated(NMDhcpClient *self, const NMPlatformIP6Address *prefix) nm_dhcp_client_emit_ipv6_prefix_delegated(NMDhcpClient *self, const NMPlatformIP6Address *prefix)
{ {
g_signal_emit(G_OBJECT(self), signals[SIGNAL_PREFIX_DELEGATED], 0, prefix); const NMDhcpClientNotifyData notify_data = {
.notify_type = NM_DHCP_CLIENT_NOTIFY_TYPE_PREFIX_DELEGATED,
.prefix_delegated =
{
.prefix = prefix,
},
};
_emit_notify(self, &notify_data);
} }
gboolean gboolean
@ -1387,28 +1418,15 @@ nm_dhcp_client_class_init(NMDhcpClientClass *client_class)
g_object_class_install_properties(object_class, _PROPERTY_ENUMS_LAST, obj_properties); g_object_class_install_properties(object_class, _PROPERTY_ENUMS_LAST, obj_properties);
signals[SIGNAL_STATE_CHANGED] = g_signal_new(NM_DHCP_CLIENT_SIGNAL_STATE_CHANGED, signals[SIGNAL_NOTIFY] =
G_OBJECT_CLASS_TYPE(object_class), g_signal_new(NM_DHCP_CLIENT_NOTIFY,
G_SIGNAL_RUN_FIRST,
0,
NULL,
NULL,
NULL,
G_TYPE_NONE,
3,
G_TYPE_UINT,
G_TYPE_OBJECT,
G_TYPE_HASH_TABLE);
signals[SIGNAL_PREFIX_DELEGATED] =
g_signal_new(NM_DHCP_CLIENT_SIGNAL_PREFIX_DELEGATED,
G_OBJECT_CLASS_TYPE(object_class), G_OBJECT_CLASS_TYPE(object_class),
G_SIGNAL_RUN_FIRST, G_SIGNAL_RUN_FIRST,
0, 0,
NULL, NULL,
NULL, NULL,
NULL, g_cclosure_marshal_VOID__POINTER,
G_TYPE_NONE, G_TYPE_NONE,
1, 1,
G_TYPE_POINTER /* const NMPlatformIP6Address *prefix */); G_TYPE_POINTER /* const NMDhcpClientNotifyData *notify_data */);
} }

22
src/core/dhcp/nm-dhcp-client.h
View file

@ -44,8 +44,7 @@
#define NM_DHCP_CLIENT_VENDOR_CLASS_IDENTIFIER "vendor-class-identifier" #define NM_DHCP_CLIENT_VENDOR_CLASS_IDENTIFIER "vendor-class-identifier"
#define NM_DHCP_CLIENT_REJECT_SERVERS "reject-servers" #define NM_DHCP_CLIENT_REJECT_SERVERS "reject-servers"
#define NM_DHCP_CLIENT_SIGNAL_STATE_CHANGED "state-changed" #define NM_DHCP_CLIENT_NOTIFY "dhcp-notify"
#define NM_DHCP_CLIENT_SIGNAL_PREFIX_DELEGATED "prefix-delegated"
typedef enum { typedef enum {
NM_DHCP_STATE_UNKNOWN = 0, NM_DHCP_STATE_UNKNOWN = 0,
@ -59,6 +58,25 @@ typedef enum {
NM_DHCP_STATE_NOOP, /* state is a non operation for NetworkManager */ NM_DHCP_STATE_NOOP, /* state is a non operation for NetworkManager */
} NMDhcpState; } NMDhcpState;
typedef enum _nm_packed {
NM_DHCP_CLIENT_NOTIFY_TYPE_STATE_CHANGED,
NM_DHCP_CLIENT_NOTIFY_TYPE_PREFIX_DELEGATED,
} NMDhcpClientNotifyType;
typedef struct {
NMDhcpClientNotifyType notify_type;
union {
struct {
NMIPConfig *ip_config;
GHashTable *options;
NMDhcpState dhcp_state;
} state_changed;
struct {
const NMPlatformIP6Address *prefix;
} prefix_delegated;
};
} NMDhcpClientNotifyData;
const char *nm_dhcp_state_to_string(NMDhcpState state); const char *nm_dhcp_state_to_string(NMDhcpState state);
struct _NMDhcpClientPrivate; struct _NMDhcpClientPrivate;

23
src/core/dhcp/nm-dhcp-manager.c
View file

@ -45,11 +45,8 @@ G_DEFINE_TYPE(NMDhcpManager, nm_dhcp_manager, G_TYPE_OBJECT)
/*****************************************************************************/ /*****************************************************************************/
static void client_state_changed(NMDhcpClient * client, static void
NMDhcpState state, client_notify(NMDhcpClient *client, const NMDhcpClientNotifyData *notify_data, NMDhcpManager *self);
GObject * ip_config,
GVariant * options,
NMDhcpManager *self);
/*****************************************************************************/ /*****************************************************************************/
@ -161,7 +158,7 @@ get_client_for_ifindex(NMDhcpManager *manager, int addr_family, int ifindex)
static void static void
remove_client(NMDhcpManager *self, NMDhcpClient *client) remove_client(NMDhcpManager *self, NMDhcpClient *client)
{ {
g_signal_handlers_disconnect_by_func(client, client_state_changed, self); g_signal_handlers_disconnect_by_func(client, client_notify, self);
c_list_unlink(&client->dhcp_client_lst); c_list_unlink(&client->dhcp_client_lst);
/* Stopping the client is left up to the controlling device /* Stopping the client is left up to the controlling device
@ -178,13 +175,10 @@ remove_client_unref(NMDhcpManager *self, NMDhcpClient *client)
} }
static void static void
client_state_changed(NMDhcpClient * client, client_notify(NMDhcpClient *client, const NMDhcpClientNotifyData *notify_data, NMDhcpManager *self)
NMDhcpState state,
GObject * ip_config,
GVariant * options,
NMDhcpManager *self)
{ {
if (state >= NM_DHCP_STATE_TIMEOUT) if (notify_data->notify_type == NM_DHCP_CLIENT_NOTIFY_TYPE_STATE_CHANGED
&& notify_data->state_changed.dhcp_state >= NM_DHCP_STATE_TIMEOUT)
remove_client_unref(self, client); remove_client_unref(self, client);
} }
@ -334,10 +328,7 @@ client_start(NMDhcpManager * self,
NULL); NULL);
nm_assert(client && c_list_is_empty(&client->dhcp_client_lst)); nm_assert(client && c_list_is_empty(&client->dhcp_client_lst));
c_list_link_tail(&priv->dhcp_client_lst_head, &client->dhcp_client_lst); c_list_link_tail(&priv->dhcp_client_lst_head, &client->dhcp_client_lst);
g_signal_connect(client, g_signal_connect(client, NM_DHCP_CLIENT_NOTIFY, G_CALLBACK(client_notify), self);
NM_DHCP_CLIENT_SIGNAL_STATE_CHANGED,
G_CALLBACK(client_state_changed),
self);
/* unfortunately, our implementations work differently per address-family regarding client-id/DUID. /* unfortunately, our implementations work differently per address-family regarding client-id/DUID.
* *

25
src/core/nm-iface-helper.c
View file

@ -92,24 +92,29 @@ static struct {
/*****************************************************************************/ /*****************************************************************************/
static void static void
dhcp4_state_changed(NMDhcpClient *client, _dhcp_client_notify_cb(NMDhcpClient * client,
NMDhcpState state, const NMDhcpClientNotifyData *notify_data,
NMIP4Config * ip4_config, gpointer user_data)
GHashTable * options,
gpointer user_data)
{ {
static NMIP4Config *last_config = NULL; static NMIP4Config *last_config = NULL;
NMIP4Config * existing; NMIP4Config * existing;
gs_unref_ptrarray GPtrArray *ip4_dev_route_blacklist = NULL; gs_unref_ptrarray GPtrArray *ip4_dev_route_blacklist = NULL;
gs_free_error GError *error = NULL; gs_free_error GError *error = NULL;
NMIP4Config * ip4_config;
g_return_if_fail(!ip4_config || NM_IS_IP4_CONFIG(ip4_config)); if (!notify_data || notify_data->notify_type != NM_DHCP_CLIENT_NOTIFY_TYPE_STATE_CHANGED)
g_return_if_reached();
_LOGD(LOGD_DHCP4, "new DHCPv4 client state %d", state); nm_assert(!notify_data->state_changed.ip_config
|| NM_IS_IP4_CONFIG(notify_data->state_changed.ip_config));
switch (state) { _LOGD(LOGD_DHCP4, "new DHCPv4 client state %d", (int) notify_data->state_changed.dhcp_state);
switch (notify_data->state_changed.dhcp_state) {
case NM_DHCP_STATE_BOUND: case NM_DHCP_STATE_BOUND:
case NM_DHCP_STATE_EXTENDED: case NM_DHCP_STATE_EXTENDED:
ip4_config = NM_IP4_CONFIG(notify_data->state_changed.ip_config);
g_assert(ip4_config); g_assert(ip4_config);
g_assert(nm_ip4_config_get_ifindex(ip4_config) == gl.ifindex); g_assert(nm_ip4_config_get_ifindex(ip4_config) == gl.ifindex);
@ -684,8 +689,8 @@ main(int argc, char *argv[])
g_error("failure to start DHCP: %s", error->message); g_error("failure to start DHCP: %s", error->message);
g_signal_connect(dhcp4_client, g_signal_connect(dhcp4_client,
NM_DHCP_CLIENT_SIGNAL_STATE_CHANGED, NM_DHCP_CLIENT_NOTIFY,
G_CALLBACK(dhcp4_state_changed), G_CALLBACK(_dhcp_client_notify_cb),
NULL); NULL);
} }