From e3d3f1315ae8b83e86ae42ae1e559ab9e47aa719 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Thu, 12 Dec 2024 21:06:10 +0100 Subject: [PATCH 01/28] device/factory: document that some callbacks get an incomplete connection It's get_connection_parent() and get_connection_iface(). --- src/core/devices/nm-device-factory.h | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/core/devices/nm-device-factory.h b/src/core/devices/nm-device-factory.h index 004ae9b1bf..d4aae75fe9 100644 --- a/src/core/devices/nm-device-factory.h +++ b/src/core/devices/nm-device-factory.h @@ -69,11 +69,15 @@ typedef struct { /** * get_connection_parent: * @factory: the #NMDeviceFactory - * @connection: the #NMConnection to return the parent name for, if supported + * @connection: the #NMConnection (possibly incomplete) to return the parent name for, if supported * * Given a connection, returns the parent interface name, parent connection * UUID, or parent device permanent hardware address for @connection. * + * Note that @connection is not necessarily a valid connection. + * It might be called during AddAndActivate before the connection is + * completed and normalized. + * * Returns: the parent interface name, parent connection UUID, parent * device permanent hardware address, or %NULL */ @@ -82,12 +86,16 @@ typedef struct { /** * get_connection_iface: * @factory: the #NMDeviceFactory - * @connection: the #NMConnection to return the interface name for + * @connection: the #NMConnection (possibly incomplete) to return the interface name for * @parent_iface: optional parent interface name for virtual devices * * Given a connection, returns the interface name that a device activating * that connection would have. * + * Note that @connection is not necessarily a valid connection. + * It might be called during AddAndActivate before the connection is + * completed and normalized. + * * Returns: the interface name, or %NULL */ char *(*get_connection_iface)(NMDeviceFactory *factory, From b7a8486c536deb4093f0f8ef3659cd1e90d6f5a6 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 13 Dec 2024 10:27:40 +0100 Subject: [PATCH 02/28] device: cleanup get_connection_iface() callbacks Some of them are wrong: they assert a connection has a particular setting even though this can be called on AddAndActivate against a connection that is not complete or normalized: impl_manager_add_and_activate_connection () validate_activation_request () nm_manager_get_best_device_for_connection () iface = nm_manager_get_connection_iface () find_parent_device_for_connection () nm_device_factory_get_connection_parent () nm_device_factory_get_connection_iface () <====== here find_device_by_iface (iface) nm_device_complete_connection () Fix those by removing the assertions. Some of them are also fall back to just calling nm_connection_get_interface_name() which is a pretty useless thing to do because nm_device_factory_get_connection_iface() only calls the device-specific routine if nm_device_factory_get_connection_iface() doesn't return anything, to give the factory a chance to make up a name (like . for Vlan) on its own. Drop those. --- src/core/devices/nm-device-6lowpan.c | 21 +-------------------- src/core/devices/nm-device-infiniband.c | 16 +++++++++------- src/core/devices/nm-device-ip-tunnel.c | 23 +---------------------- src/core/devices/nm-device-ipvlan.c | 21 +-------------------- src/core/devices/nm-device-macsec.c | 21 +-------------------- src/core/devices/nm-device-macvlan.c | 21 +-------------------- src/core/devices/nm-device-ppp.c | 12 ------------ src/core/devices/nm-device-vlan.c | 18 +++++------------- src/core/devices/nm-device-vxlan.c | 21 +-------------------- 9 files changed, 20 insertions(+), 154 deletions(-) diff --git a/src/core/devices/nm-device-6lowpan.c b/src/core/devices/nm-device-6lowpan.c index 78ec634c24..ab35b3e9e8 100644 --- a/src/core/devices/nm-device-6lowpan.c +++ b/src/core/devices/nm-device-6lowpan.c @@ -281,24 +281,6 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) return nm_setting_6lowpan_get_parent(s_6lowpan); } -static char * -get_connection_iface(NMDeviceFactory *factory, NMConnection *connection, const char *parent_iface) -{ - NMSetting6Lowpan *s_6lowpan; - const char *ifname; - - g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_6LOWPAN_SETTING_NAME), NULL); - - s_6lowpan = NM_SETTING_6LOWPAN(nm_connection_get_setting(connection, NM_TYPE_SETTING_6LOWPAN)); - g_assert(s_6lowpan); - - if (!parent_iface) - return NULL; - - ifname = nm_connection_get_interface_name(connection); - return g_strdup(ifname); -} - NM_DEVICE_FACTORY_DEFINE_INTERNAL( 6LOWPAN, 6Lowpan, @@ -306,5 +288,4 @@ NM_DEVICE_FACTORY_DEFINE_INTERNAL( NM_DEVICE_FACTORY_DECLARE_LINK_TYPES(NM_LINK_TYPE_6LOWPAN) NM_DEVICE_FACTORY_DECLARE_SETTING_TYPES(NM_SETTING_6LOWPAN_SETTING_NAME), factory_class->create_device = create_device; - factory_class->get_connection_parent = get_connection_parent; - factory_class->get_connection_iface = get_connection_iface;); + factory_class->get_connection_parent = get_connection_parent;); diff --git a/src/core/devices/nm-device-infiniband.c b/src/core/devices/nm-device-infiniband.c index c974696c42..ff4f9e0a4d 100644 --- a/src/core/devices/nm-device-infiniband.c +++ b/src/core/devices/nm-device-infiniband.c @@ -477,17 +477,19 @@ get_connection_iface(NMDeviceFactory *factory, NMConnection *connection, const c g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_INFINIBAND_SETTING_NAME), NULL); - s_infiniband = nm_connection_get_setting_infiniband(connection); - g_assert(s_infiniband); - if (!parent_iface) return NULL; - g_return_val_if_fail(g_strcmp0(parent_iface, nm_setting_infiniband_get_parent(s_infiniband)) - == 0, - NULL); + s_infiniband = nm_connection_get_setting_infiniband(connection); + if (s_infiniband) { + g_return_val_if_fail(g_strcmp0(parent_iface, nm_setting_infiniband_get_parent(s_infiniband)) + == 0, + NULL); - return g_strdup(nm_setting_infiniband_get_virtual_interface_name(s_infiniband)); + return g_strdup(nm_setting_infiniband_get_virtual_interface_name(s_infiniband)); + } else { + return NULL; + } } NM_DEVICE_FACTORY_DEFINE_INTERNAL( diff --git a/src/core/devices/nm-device-ip-tunnel.c b/src/core/devices/nm-device-ip-tunnel.c index cc62180e40..8ec671f233 100644 --- a/src/core/devices/nm-device-ip-tunnel.c +++ b/src/core/devices/nm-device-ip-tunnel.c @@ -1374,26 +1374,6 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) return nm_setting_ip_tunnel_get_parent(s_ip_tunnel); } -static char * -get_connection_iface(NMDeviceFactory *factory, NMConnection *connection, const char *parent_iface) -{ - const char *ifname; - NMSettingIPTunnel *s_ip_tunnel; - - g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_IP_TUNNEL_SETTING_NAME), - NULL); - - s_ip_tunnel = nm_connection_get_setting_ip_tunnel(connection); - g_assert(s_ip_tunnel); - - if (nm_setting_ip_tunnel_get_parent(s_ip_tunnel) && !parent_iface) - return NULL; - - ifname = nm_connection_get_interface_name(connection); - - return g_strdup(ifname); -} - NM_DEVICE_FACTORY_DEFINE_INTERNAL( IP_TUNNEL, IPTunnel, @@ -1409,5 +1389,4 @@ NM_DEVICE_FACTORY_DEFINE_INTERNAL( NM_LINK_TYPE_VTI6) NM_DEVICE_FACTORY_DECLARE_SETTING_TYPES(NM_SETTING_IP_TUNNEL_SETTING_NAME), factory_class->create_device = create_device; - factory_class->get_connection_parent = get_connection_parent; - factory_class->get_connection_iface = get_connection_iface;); + factory_class->get_connection_parent = get_connection_parent;); diff --git a/src/core/devices/nm-device-ipvlan.c b/src/core/devices/nm-device-ipvlan.c index 662e3ff28b..068cfa6dc3 100644 --- a/src/core/devices/nm-device-ipvlan.c +++ b/src/core/devices/nm-device-ipvlan.c @@ -458,24 +458,6 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) return NULL; } -static char * -get_connection_iface(NMDeviceFactory *factory, NMConnection *connection, const char *parent_iface) -{ - NMSettingIpvlan *s_ipvlan; - const char *ifname; - - g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_IPVLAN_SETTING_NAME), NULL); - - s_ipvlan = _nm_connection_get_setting(connection, NM_TYPE_SETTING_IPVLAN); - nm_assert(s_ipvlan); - - if (!parent_iface) - return NULL; - - ifname = nm_connection_get_interface_name(connection); - return g_strdup(ifname); -} - NM_DEVICE_FACTORY_DEFINE_INTERNAL( IPVLAN, Ipvlan, @@ -483,5 +465,4 @@ NM_DEVICE_FACTORY_DEFINE_INTERNAL( NM_DEVICE_FACTORY_DECLARE_LINK_TYPES(NM_LINK_TYPE_IPVLAN) NM_DEVICE_FACTORY_DECLARE_SETTING_TYPES(NM_SETTING_IPVLAN_SETTING_NAME), factory_class->create_device = create_device; - factory_class->get_connection_parent = get_connection_parent; - factory_class->get_connection_iface = get_connection_iface;); + factory_class->get_connection_parent = get_connection_parent;); diff --git a/src/core/devices/nm-device-macsec.c b/src/core/devices/nm-device-macsec.c index 32fab5be63..6788c6df23 100644 --- a/src/core/devices/nm-device-macsec.c +++ b/src/core/devices/nm-device-macsec.c @@ -1036,24 +1036,6 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) return NULL; } -static char * -get_connection_iface(NMDeviceFactory *factory, NMConnection *connection, const char *parent_iface) -{ - NMSettingMacsec *s_macsec; - const char *ifname; - - g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_MACSEC_SETTING_NAME), NULL); - - s_macsec = nm_connection_get_setting_macsec(connection); - g_assert(s_macsec); - - if (!parent_iface) - return NULL; - - ifname = nm_connection_get_interface_name(connection); - return g_strdup(ifname); -} - NM_DEVICE_FACTORY_DEFINE_INTERNAL( MACSEC, Macsec, @@ -1061,5 +1043,4 @@ NM_DEVICE_FACTORY_DEFINE_INTERNAL( NM_DEVICE_FACTORY_DECLARE_LINK_TYPES(NM_LINK_TYPE_MACSEC) NM_DEVICE_FACTORY_DECLARE_SETTING_TYPES(NM_SETTING_MACSEC_SETTING_NAME), factory_class->create_device = create_device; - factory_class->get_connection_parent = get_connection_parent; - factory_class->get_connection_iface = get_connection_iface;) + factory_class->get_connection_parent = get_connection_parent;); diff --git a/src/core/devices/nm-device-macvlan.c b/src/core/devices/nm-device-macvlan.c index 8cdef0cfe2..47bad54c9d 100644 --- a/src/core/devices/nm-device-macvlan.c +++ b/src/core/devices/nm-device-macvlan.c @@ -604,24 +604,6 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) return NULL; } -static char * -get_connection_iface(NMDeviceFactory *factory, NMConnection *connection, const char *parent_iface) -{ - NMSettingMacvlan *s_macvlan; - const char *ifname; - - g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_MACVLAN_SETTING_NAME), NULL); - - s_macvlan = nm_connection_get_setting_macvlan(connection); - g_assert(s_macvlan); - - if (!parent_iface) - return NULL; - - ifname = nm_connection_get_interface_name(connection); - return g_strdup(ifname); -} - NM_DEVICE_FACTORY_DEFINE_INTERNAL( MACVLAN, Macvlan, @@ -629,5 +611,4 @@ NM_DEVICE_FACTORY_DEFINE_INTERNAL( NM_DEVICE_FACTORY_DECLARE_LINK_TYPES(NM_LINK_TYPE_MACVLAN, NM_LINK_TYPE_MACVTAP) NM_DEVICE_FACTORY_DECLARE_SETTING_TYPES(NM_SETTING_MACVLAN_SETTING_NAME), factory_class->create_device = create_device; - factory_class->get_connection_parent = get_connection_parent; - factory_class->get_connection_iface = get_connection_iface;); + factory_class->get_connection_parent = get_connection_parent;); diff --git a/src/core/devices/nm-device-ppp.c b/src/core/devices/nm-device-ppp.c index 507b9eb6c8..88150d2d5f 100644 --- a/src/core/devices/nm-device-ppp.c +++ b/src/core/devices/nm-device-ppp.c @@ -385,17 +385,6 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) return nm_setting_pppoe_get_parent(s_pppoe); } -static char * -get_connection_iface(NMDeviceFactory *factory, NMConnection *connection, const char *parent_iface) -{ - nm_assert(nm_connection_is_type(connection, NM_SETTING_PPPOE_SETTING_NAME)); - - if (!parent_iface) - return NULL; - - return g_strdup(nm_connection_get_interface_name(connection)); -} - NM_DEVICE_FACTORY_DEFINE_INTERNAL( PPP, Ppp, @@ -403,6 +392,5 @@ NM_DEVICE_FACTORY_DEFINE_INTERNAL( NM_DEVICE_FACTORY_DECLARE_LINK_TYPES(NM_LINK_TYPE_PPP) NM_DEVICE_FACTORY_DECLARE_SETTING_TYPES(NM_SETTING_PPPOE_SETTING_NAME), factory_class->get_connection_parent = get_connection_parent; - factory_class->get_connection_iface = get_connection_iface; factory_class->create_device = create_device; factory_class->match_connection = match_connection;); diff --git a/src/core/devices/nm-device-vlan.c b/src/core/devices/nm-device-vlan.c index a2c5a27b9f..5b901189d6 100644 --- a/src/core/devices/nm-device-vlan.c +++ b/src/core/devices/nm-device-vlan.c @@ -635,26 +635,18 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) static char * get_connection_iface(NMDeviceFactory *factory, NMConnection *connection, const char *parent_iface) { - const char *ifname; NMSettingVlan *s_vlan; g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_VLAN_SETTING_NAME), NULL); - s_vlan = nm_connection_get_setting_vlan(connection); - g_assert(s_vlan); - if (!parent_iface) return NULL; - ifname = nm_connection_get_interface_name(connection); - if (ifname) - return g_strdup(ifname); - - /* If the connection doesn't specify the interface name for the VLAN - * device, we create one for it using the VLAN ID and the parent - * interface's name. - */ - return nmp_utils_new_vlan_name(parent_iface, nm_setting_vlan_get_id(s_vlan)); + s_vlan = nm_connection_get_setting_vlan(connection); + if (s_vlan) + return nmp_utils_new_vlan_name(parent_iface, nm_setting_vlan_get_id(s_vlan)); + else + return NULL; } NM_DEVICE_FACTORY_DEFINE_INTERNAL( diff --git a/src/core/devices/nm-device-vxlan.c b/src/core/devices/nm-device-vxlan.c index 6a23d51e69..6ab2d6a1a9 100644 --- a/src/core/devices/nm-device-vxlan.c +++ b/src/core/devices/nm-device-vxlan.c @@ -782,24 +782,6 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) return nm_setting_vxlan_get_parent(s_vxlan); } -static char * -get_connection_iface(NMDeviceFactory *factory, NMConnection *connection, const char *parent_iface) -{ - const char *ifname; - NMSettingVxlan *s_vxlan; - - g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_VXLAN_SETTING_NAME), NULL); - - s_vxlan = nm_connection_get_setting_vxlan(connection); - g_assert(s_vxlan); - - if (nm_setting_vxlan_get_parent(s_vxlan) && !parent_iface) - return NULL; - - ifname = nm_connection_get_interface_name(connection); - return g_strdup(ifname); -} - NM_DEVICE_FACTORY_DEFINE_INTERNAL( VXLAN, Vxlan, @@ -807,5 +789,4 @@ NM_DEVICE_FACTORY_DEFINE_INTERNAL( NM_DEVICE_FACTORY_DECLARE_LINK_TYPES(NM_LINK_TYPE_VXLAN) NM_DEVICE_FACTORY_DECLARE_SETTING_TYPES(NM_SETTING_VXLAN_SETTING_NAME), factory_class->create_device = create_device; - factory_class->get_connection_parent = get_connection_parent; - factory_class->get_connection_iface = get_connection_iface;); + factory_class->get_connection_parent = get_connection_parent;); From 6635aeed99b8a41c246e6e9980e38652a6ab781d Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 13 Dec 2024 08:13:27 +0100 Subject: [PATCH 03/28] device: get_connection_parent() accept incomplete connections All of these are wrong asserting that a connection has a particular setting. On AddAndActivate, the connection can be pretty much empty: impl_manager_add_and_activate_connection () validate_activation_request () nm_manager_get_best_device_for_connection () iface = nm_manager_get_connection_iface () find_parent_device_for_connection () nm_device_factory_get_connection_parent () <====== *shriek* nm_device_factory_get_connection_iface () find_device_by_iface (iface) nm_device_complete_connection () Remove those assertions. --- src/core/devices/nm-device-6lowpan.c | 7 ++++--- src/core/devices/nm-device-infiniband.c | 7 ++++--- src/core/devices/nm-device-ip-tunnel.c | 7 ++++--- src/core/devices/nm-device-ipvlan.c | 14 +++++++------- src/core/devices/nm-device-macsec.c | 14 +++++++------- src/core/devices/nm-device-macvlan.c | 14 +++++++------- src/core/devices/nm-device-ppp.c | 7 ++++--- src/core/devices/nm-device-vlan.c | 14 +++++++------- src/core/devices/nm-device-vxlan.c | 7 ++++--- 9 files changed, 48 insertions(+), 43 deletions(-) diff --git a/src/core/devices/nm-device-6lowpan.c b/src/core/devices/nm-device-6lowpan.c index ab35b3e9e8..f4b8791cbc 100644 --- a/src/core/devices/nm-device-6lowpan.c +++ b/src/core/devices/nm-device-6lowpan.c @@ -276,9 +276,10 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_6LOWPAN_SETTING_NAME), NULL); s_6lowpan = NM_SETTING_6LOWPAN(nm_connection_get_setting(connection, NM_TYPE_SETTING_6LOWPAN)); - g_assert(s_6lowpan); - - return nm_setting_6lowpan_get_parent(s_6lowpan); + if (s_6lowpan) + return nm_setting_6lowpan_get_parent(s_6lowpan); + else + return NULL; } NM_DEVICE_FACTORY_DEFINE_INTERNAL( diff --git a/src/core/devices/nm-device-infiniband.c b/src/core/devices/nm-device-infiniband.c index ff4f9e0a4d..be6ae583d5 100644 --- a/src/core/devices/nm-device-infiniband.c +++ b/src/core/devices/nm-device-infiniband.c @@ -464,9 +464,10 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) NULL); s_infiniband = nm_connection_get_setting_infiniband(connection); - g_assert(s_infiniband); - - return nm_setting_infiniband_get_parent(s_infiniband); + if (s_infiniband) + return nm_setting_infiniband_get_parent(s_infiniband); + else + return NULL; } static char * diff --git a/src/core/devices/nm-device-ip-tunnel.c b/src/core/devices/nm-device-ip-tunnel.c index 8ec671f233..26d83e19d2 100644 --- a/src/core/devices/nm-device-ip-tunnel.c +++ b/src/core/devices/nm-device-ip-tunnel.c @@ -1369,9 +1369,10 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) NULL); s_ip_tunnel = nm_connection_get_setting_ip_tunnel(connection); - g_assert(s_ip_tunnel); - - return nm_setting_ip_tunnel_get_parent(s_ip_tunnel); + if (s_ip_tunnel) + return nm_setting_ip_tunnel_get_parent(s_ip_tunnel); + else + return NULL; } NM_DEVICE_FACTORY_DEFINE_INTERNAL( diff --git a/src/core/devices/nm-device-ipvlan.c b/src/core/devices/nm-device-ipvlan.c index 068cfa6dc3..6ca20b9d28 100644 --- a/src/core/devices/nm-device-ipvlan.c +++ b/src/core/devices/nm-device-ipvlan.c @@ -444,18 +444,18 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_IPVLAN_SETTING_NAME), NULL); s_ipvlan = _nm_connection_get_setting(connection, NM_TYPE_SETTING_IPVLAN); - nm_assert(s_ipvlan); - - parent = nm_setting_ipvlan_get_parent(s_ipvlan); - if (parent) - return parent; + if (s_ipvlan) { + parent = nm_setting_ipvlan_get_parent(s_ipvlan); + if (parent) + return parent; + } /* Try the hardware address from the IPVLAN connection's hardware setting */ s_wired = nm_connection_get_setting_wired(connection); if (s_wired) return nm_setting_wired_get_mac_address(s_wired); - - return NULL; + else + return NULL; } NM_DEVICE_FACTORY_DEFINE_INTERNAL( diff --git a/src/core/devices/nm-device-macsec.c b/src/core/devices/nm-device-macsec.c index 6788c6df23..89a0672097 100644 --- a/src/core/devices/nm-device-macsec.c +++ b/src/core/devices/nm-device-macsec.c @@ -1022,18 +1022,18 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_MACSEC_SETTING_NAME), NULL); s_macsec = nm_connection_get_setting_macsec(connection); - g_assert(s_macsec); - - parent = nm_setting_macsec_get_parent(s_macsec); - if (parent) - return parent; + if (s_macsec) { + parent = nm_setting_macsec_get_parent(s_macsec); + if (parent) + return parent; + } /* Try the hardware address from the MACsec connection's hardware setting */ s_wired = nm_connection_get_setting_wired(connection); if (s_wired) return nm_setting_wired_get_mac_address(s_wired); - - return NULL; + else + return NULL; } NM_DEVICE_FACTORY_DEFINE_INTERNAL( diff --git a/src/core/devices/nm-device-macvlan.c b/src/core/devices/nm-device-macvlan.c index 47bad54c9d..daf2554da7 100644 --- a/src/core/devices/nm-device-macvlan.c +++ b/src/core/devices/nm-device-macvlan.c @@ -590,18 +590,18 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_MACVLAN_SETTING_NAME), NULL); s_macvlan = nm_connection_get_setting_macvlan(connection); - g_assert(s_macvlan); - - parent = nm_setting_macvlan_get_parent(s_macvlan); - if (parent) - return parent; + if (s_macvlan) { + parent = nm_setting_macvlan_get_parent(s_macvlan); + if (parent) + return parent; + } /* Try the hardware address from the MACVLAN connection's hardware setting */ s_wired = nm_connection_get_setting_wired(connection); if (s_wired) return nm_setting_wired_get_mac_address(s_wired); - - return NULL; + else + return NULL; } NM_DEVICE_FACTORY_DEFINE_INTERNAL( diff --git a/src/core/devices/nm-device-ppp.c b/src/core/devices/nm-device-ppp.c index 88150d2d5f..f44fe2f0c3 100644 --- a/src/core/devices/nm-device-ppp.c +++ b/src/core/devices/nm-device-ppp.c @@ -380,9 +380,10 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) nm_assert(nm_connection_is_type(connection, NM_SETTING_PPPOE_SETTING_NAME)); s_pppoe = nm_connection_get_setting_pppoe(connection); - nm_assert(s_pppoe); - - return nm_setting_pppoe_get_parent(s_pppoe); + if (s_pppoe) + return nm_setting_pppoe_get_parent(s_pppoe); + else + return NULL; } NM_DEVICE_FACTORY_DEFINE_INTERNAL( diff --git a/src/core/devices/nm-device-vlan.c b/src/core/devices/nm-device-vlan.c index 5b901189d6..08e6c30afc 100644 --- a/src/core/devices/nm-device-vlan.c +++ b/src/core/devices/nm-device-vlan.c @@ -618,18 +618,18 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_VLAN_SETTING_NAME), NULL); s_vlan = nm_connection_get_setting_vlan(connection); - g_assert(s_vlan); - - parent = nm_setting_vlan_get_parent(s_vlan); - if (parent) - return parent; + if (s_vlan) { + parent = nm_setting_vlan_get_parent(s_vlan); + if (parent) + return parent; + } /* Try the hardware address from the VLAN connection's hardware setting */ s_wired = nm_connection_get_setting_wired(connection); if (s_wired) return nm_setting_wired_get_mac_address(s_wired); - - return NULL; + else + return NULL; } static char * diff --git a/src/core/devices/nm-device-vxlan.c b/src/core/devices/nm-device-vxlan.c index 6ab2d6a1a9..9c54791462 100644 --- a/src/core/devices/nm-device-vxlan.c +++ b/src/core/devices/nm-device-vxlan.c @@ -777,9 +777,10 @@ get_connection_parent(NMDeviceFactory *factory, NMConnection *connection) g_return_val_if_fail(nm_connection_is_type(connection, NM_SETTING_VXLAN_SETTING_NAME), NULL); s_vxlan = nm_connection_get_setting_vxlan(connection); - g_assert(s_vxlan); - - return nm_setting_vxlan_get_parent(s_vxlan); + if (s_vxlan) + return nm_setting_vxlan_get_parent(s_vxlan); + else + return NULL; } NM_DEVICE_FACTORY_DEFINE_INTERNAL( From be034a1f3f2fc757ff011f054abb1606e9e8febc Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 7 Jan 2025 08:28:01 +0100 Subject: [PATCH 04/28] device: simplify the nm_utils_complete_generic() machinery The point is to get rid of device/connection type specific arguments, to eventually be able to complete the connection on AddAndActivate before knowing which factory is going to take care of creating the device. Aside from that, the whole thing is pretty awful -- with complicated macros and variadic argument (ugh). Let's get rid of that. --- src/core/NetworkManagerUtils.c | 48 ++++++++-------- src/core/NetworkManagerUtils.h | 62 +++------------------ src/core/devices/adsl/nm-device-adsl.c | 3 +- src/core/devices/bluetooth/nm-device-bt.c | 3 +- src/core/devices/nm-device-6lowpan.c | 3 +- src/core/devices/nm-device-bond.c | 3 +- src/core/devices/nm-device-bridge.c | 3 +- src/core/devices/nm-device-dummy.c | 16 +++--- src/core/devices/nm-device-ethernet.c | 6 +- src/core/devices/nm-device-infiniband.c | 3 +- src/core/devices/nm-device-ip-tunnel.c | 3 +- src/core/devices/nm-device-loopback.c | 16 +++--- src/core/devices/nm-device-macvlan.c | 3 +- src/core/devices/nm-device-tun.c | 3 +- src/core/devices/nm-device-vlan.c | 3 +- src/core/devices/nm-device-vrf.c | 3 +- src/core/devices/nm-device-vxlan.c | 3 +- src/core/devices/nm-device-wpan.c | 3 +- src/core/devices/team/nm-device-team.c | 3 +- src/core/devices/wifi/nm-device-iwd-p2p.c | 3 +- src/core/devices/wifi/nm-device-iwd.c | 3 +- src/core/devices/wifi/nm-device-olpc-mesh.c | 3 +- src/core/devices/wifi/nm-device-wifi-p2p.c | 3 +- src/core/devices/wifi/nm-device-wifi.c | 3 +- src/core/devices/wwan/nm-modem-broadband.c | 6 +- src/core/nm-manager.c | 3 +- 26 files changed, 71 insertions(+), 143 deletions(-) diff --git a/src/core/NetworkManagerUtils.c b/src/core/NetworkManagerUtils.c index d4180b0bc2..6c9e2f3d19 100644 --- a/src/core/NetworkManagerUtils.c +++ b/src/core/NetworkManagerUtils.c @@ -250,23 +250,19 @@ nm_utils_ppp_ip_methods_enabled(NMConnection *connection, /*****************************************************************************/ void -_nm_utils_complete_generic_with_params(NMPlatform *platform, - NMConnection *connection, - const char *ctype, - NMConnection *const *existing_connections, - const char *preferred_id, - const char *fallback_id_prefix, - const char *ifname_prefix, - const char *ifname, - ...) +nm_utils_complete_generic(NMPlatform *platform, + NMConnection *connection, + const char *ctype, + NMConnection *const *existing_connections, + const char *preferred_id, + const char *fallback_id_prefix, + const char *ifname_prefix, + const char *ifname) { NMSettingConnection *s_con; char *id; char *generated_ifname; gs_unref_hashtable GHashTable *parameters = NULL; - va_list ap; - const char *p_val; - const char *p_key; g_assert(fallback_id_prefix); g_return_if_fail(ifname_prefix == NULL || ifname == NULL); @@ -301,20 +297,22 @@ _nm_utils_complete_generic_with_params(NMPlatform *platform, g_free(generated_ifname); } - /* Normalize */ - va_start(ap, ifname); - while ((p_key = va_arg(ap, const char *))) { - p_val = va_arg(ap, const char *); - if (!p_val) { - if (parameters) - g_hash_table_remove(parameters, p_key); - continue; - } - if (!parameters) - parameters = g_hash_table_new(nm_str_hash, g_str_equal); - g_hash_table_insert(parameters, (char *) p_key, (char *) p_val); + if (nm_connection_get_setting_adsl(connection) || nm_connection_get_setting_cdma(connection) + || nm_connection_get_setting_olpc_mesh(connection) + || nm_connection_get_setting_pppoe(connection) + || nm_connection_get_setting_vpn(connection)) { + parameters = g_hash_table_new(nm_str_hash, g_str_equal); + g_hash_table_insert(parameters, + NM_CONNECTION_NORMALIZE_PARAM_IP6_CONFIG_METHOD, + NM_SETTING_IP6_CONFIG_METHOD_IGNORE); + } else { + parameters = NULL; } - va_end(ap); + + /* We ignore the result, because the caller validates the connection. + * The only reason we do a normalization attempt here is + * NM_CONNECTION_NORMALIZE_PARAM_IP6_CONFIG_METHOD. + * Could we perhaps, one day, get rid of it? */ nm_connection_normalize(connection, parameters, NULL, NULL); } diff --git a/src/core/NetworkManagerUtils.h b/src/core/NetworkManagerUtils.h index 7d8afe5a2b..01f5bdb07d 100644 --- a/src/core/NetworkManagerUtils.h +++ b/src/core/NetworkManagerUtils.h @@ -23,60 +23,14 @@ void nm_utils_ppp_ip_methods_enabled(NMConnection *connection, gboolean *out_ip4_enabled, gboolean *out_ip6_enabled); -void _nm_utils_complete_generic_with_params(NMPlatform *platform, - NMConnection *connection, - const char *ctype, - NMConnection *const *existing_connections, - const char *preferred_id, - const char *fallback_id_prefix, - const char *ifname_prefix, - const char *ifname, - ...) G_GNUC_NULL_TERMINATED; - -#define nm_utils_complete_generic_with_params(platform, \ - connection, \ - ctype, \ - existing_connections, \ - preferred_id, \ - fallback_id_prefix, \ - ifname_prefix, \ - ifname, \ - ...) \ - _nm_utils_complete_generic_with_params(platform, \ - connection, \ - ctype, \ - existing_connections, \ - preferred_id, \ - fallback_id_prefix, \ - ifname_prefix, \ - ifname, \ - ##__VA_ARGS__, \ - NULL) - -static inline void -nm_utils_complete_generic(NMPlatform *platform, - NMConnection *connection, - const char *ctype, - NMConnection *const *existing_connections, - const char *preferred_id, - const char *fallback_id_prefix, - const char *ifname_prefix, - const char *ifname, - gboolean default_enable_ipv6) -{ - nm_utils_complete_generic_with_params(platform, - connection, - ctype, - existing_connections, - preferred_id, - fallback_id_prefix, - ifname_prefix, - ifname, - NM_CONNECTION_NORMALIZE_PARAM_IP6_CONFIG_METHOD, - default_enable_ipv6 - ? NM_SETTING_IP6_CONFIG_METHOD_AUTO - : NM_SETTING_IP6_CONFIG_METHOD_IGNORE); -} +void nm_utils_complete_generic(NMPlatform *platform, + NMConnection *connection, + const char *ctype, + NMConnection *const *existing_connections, + const char *preferred_id, + const char *fallback_id_prefix, + const char *ifname_prefix, + const char *ifname); typedef gboolean(NMUtilsMatchFilterFunc)(NMConnection *connection, gpointer user_data); diff --git a/src/core/devices/adsl/nm-device-adsl.c b/src/core/devices/adsl/nm-device-adsl.c index 5c3432b286..a6dc63269c 100644 --- a/src/core/devices/adsl/nm-device-adsl.c +++ b/src/core/devices/adsl/nm-device-adsl.c @@ -117,8 +117,7 @@ complete_connection(NMDevice *device, NULL, _("ADSL connection"), NULL, - NULL, - FALSE); /* No IPv6 yet by default */ + NULL); return TRUE; } diff --git a/src/core/devices/bluetooth/nm-device-bt.c b/src/core/devices/bluetooth/nm-device-bt.c index ce110aa0e6..4406bcf4eb 100644 --- a/src/core/devices/bluetooth/nm-device-bt.c +++ b/src/core/devices/bluetooth/nm-device-bt.c @@ -404,8 +404,7 @@ complete_connection(NMDevice *device, preferred, fallback_prefix, NULL, - NULL, - is_dun ? FALSE : TRUE); /* No IPv6 yet for DUN */ + NULL); setting_bdaddr = nm_setting_bluetooth_get_bdaddr(s_bt); if (setting_bdaddr) { diff --git a/src/core/devices/nm-device-6lowpan.c b/src/core/devices/nm-device-6lowpan.c index f4b8791cbc..3dabcb9bdc 100644 --- a/src/core/devices/nm-device-6lowpan.c +++ b/src/core/devices/nm-device-6lowpan.c @@ -161,8 +161,7 @@ complete_connection(NMDevice *device, NULL, _("6LOWPAN connection"), NULL, - NULL, - TRUE); + NULL); s_6lowpan = NM_SETTING_6LOWPAN(nm_connection_get_setting(connection, NM_TYPE_SETTING_6LOWPAN)); if (!s_6lowpan) { diff --git a/src/core/devices/nm-device-bond.c b/src/core/devices/nm-device-bond.c index 49ca156d5f..53b324660e 100644 --- a/src/core/devices/nm-device-bond.c +++ b/src/core/devices/nm-device-bond.c @@ -94,8 +94,7 @@ complete_connection(NMDevice *device, NULL, _("Bond connection"), "bond", - NULL, - TRUE); + NULL); _nm_connection_ensure_setting(connection, NM_TYPE_SETTING_BOND); diff --git a/src/core/devices/nm-device-bridge.c b/src/core/devices/nm-device-bridge.c index 6f06dbf490..7c34fde07f 100644 --- a/src/core/devices/nm-device-bridge.c +++ b/src/core/devices/nm-device-bridge.c @@ -164,8 +164,7 @@ complete_connection(NMDevice *device, NULL, _("Bridge connection"), "bridge", - NULL, - TRUE); + NULL); _nm_connection_ensure_setting(connection, NM_TYPE_SETTING_BRIDGE); diff --git a/src/core/devices/nm-device-dummy.c b/src/core/devices/nm-device-dummy.c index b7c4106a0a..1bc5447f5c 100644 --- a/src/core/devices/nm-device-dummy.c +++ b/src/core/devices/nm-device-dummy.c @@ -48,14 +48,14 @@ complete_connection(NMDevice *device, NMConnection *const *existing_connections, GError **error) { - nm_utils_complete_generic_with_params(nm_device_get_platform(device), - connection, - NM_SETTING_DUMMY_SETTING_NAME, - existing_connections, - NULL, - _("Dummy connection"), - NULL, - nm_device_get_ip_iface(device)); + nm_utils_complete_generic(nm_device_get_platform(device), + connection, + NM_SETTING_DUMMY_SETTING_NAME, + existing_connections, + NULL, + _("Dummy connection"), + NULL, + nm_device_get_ip_iface(device)); _nm_connection_ensure_setting(connection, NM_TYPE_SETTING_DUMMY); diff --git a/src/core/devices/nm-device-ethernet.c b/src/core/devices/nm-device-ethernet.c index fc47e1385c..4034fdaad6 100644 --- a/src/core/devices/nm-device-ethernet.c +++ b/src/core/devices/nm-device-ethernet.c @@ -1640,8 +1640,7 @@ complete_connection(NMDevice *device, NULL, _("Veth connection"), "veth", - NULL, - TRUE); + NULL); s_veth = _nm_connection_ensure_setting(connection, NM_TYPE_SETTING_VETH); @@ -1698,8 +1697,7 @@ complete_connection(NMDevice *device, NULL, s_pppoe ? _("PPPoE connection") : _("Wired connection"), NULL, - nm_setting_wired_get_mac_address(s_wired) ? NULL : nm_device_get_iface(device), - s_pppoe ? FALSE : TRUE); /* No IPv6 by default yet for PPPoE */ + nm_setting_wired_get_mac_address(s_wired) ? NULL : nm_device_get_iface(device)); return TRUE; } diff --git a/src/core/devices/nm-device-infiniband.c b/src/core/devices/nm-device-infiniband.c index be6ae583d5..a5a82b94f7 100644 --- a/src/core/devices/nm-device-infiniband.c +++ b/src/core/devices/nm-device-infiniband.c @@ -159,8 +159,7 @@ complete_connection(NMDevice *device, NULL, _("InfiniBand connection"), NULL, - nm_setting_infiniband_get_mac_address(s_infiniband) ? NULL : nm_device_get_iface(device), - TRUE); + nm_setting_infiniband_get_mac_address(s_infiniband) ? NULL : nm_device_get_iface(device)); if (!nm_setting_infiniband_get_transport_mode(s_infiniband)) g_object_set(G_OBJECT(s_infiniband), diff --git a/src/core/devices/nm-device-ip-tunnel.c b/src/core/devices/nm-device-ip-tunnel.c index 26d83e19d2..2ecfe4531f 100644 --- a/src/core/devices/nm-device-ip-tunnel.c +++ b/src/core/devices/nm-device-ip-tunnel.c @@ -402,8 +402,7 @@ complete_connection(NMDevice *device, NULL, _("IP tunnel connection"), NULL, - NULL, - TRUE); + NULL); s_ip_tunnel = nm_connection_get_setting_ip_tunnel(connection); if (!s_ip_tunnel) { diff --git a/src/core/devices/nm-device-loopback.c b/src/core/devices/nm-device-loopback.c index ec72aa963d..268e1fb1e9 100644 --- a/src/core/devices/nm-device-loopback.c +++ b/src/core/devices/nm-device-loopback.c @@ -59,14 +59,14 @@ complete_connection(NMDevice *device, NMConnection *const *existing_connections, GError **error) { - nm_utils_complete_generic_with_params(nm_device_get_platform(device), - connection, - NM_SETTING_LOOPBACK_SETTING_NAME, - existing_connections, - NULL, - _("Loopback connection"), - NULL, - nm_device_get_ip_iface(device)); + nm_utils_complete_generic(nm_device_get_platform(device), + connection, + NM_SETTING_LOOPBACK_SETTING_NAME, + existing_connections, + NULL, + _("Loopback connection"), + NULL, + nm_device_get_ip_iface(device)); _nm_connection_ensure_setting(connection, NM_TYPE_SETTING_LOOPBACK); diff --git a/src/core/devices/nm-device-macvlan.c b/src/core/devices/nm-device-macvlan.c index daf2554da7..8300023662 100644 --- a/src/core/devices/nm-device-macvlan.c +++ b/src/core/devices/nm-device-macvlan.c @@ -365,8 +365,7 @@ complete_connection(NMDevice *device, NULL, _("MACVLAN connection"), NULL, - NULL, - TRUE); + NULL); s_macvlan = nm_connection_get_setting_macvlan(connection); if (!s_macvlan) { diff --git a/src/core/devices/nm-device-tun.c b/src/core/devices/nm-device-tun.c index 28b03cec12..faab86d0a9 100644 --- a/src/core/devices/nm-device-tun.c +++ b/src/core/devices/nm-device-tun.c @@ -143,8 +143,7 @@ complete_connection(NMDevice *device, NULL, _("TUN connection"), NULL, - NULL, - TRUE); + NULL); s_tun = nm_connection_get_setting_tun(connection); if (!s_tun) { diff --git a/src/core/devices/nm-device-vlan.c b/src/core/devices/nm-device-vlan.c index 08e6c30afc..59a429ca7f 100644 --- a/src/core/devices/nm-device-vlan.c +++ b/src/core/devices/nm-device-vlan.c @@ -379,8 +379,7 @@ complete_connection(NMDevice *device, NULL, _("VLAN connection"), NULL, - NULL, - TRUE); + NULL); s_vlan = nm_connection_get_setting_vlan(connection); if (!s_vlan) { diff --git a/src/core/devices/nm-device-vrf.c b/src/core/devices/nm-device-vrf.c index 48365ac68d..7dfd6504a1 100644 --- a/src/core/devices/nm-device-vrf.c +++ b/src/core/devices/nm-device-vrf.c @@ -184,8 +184,7 @@ complete_connection(NMDevice *device, NULL, _("VRF connection"), NULL, - NULL, - TRUE); + NULL); s_vrf = _nm_connection_get_setting(connection, NM_TYPE_SETTING_VRF); if (!s_vrf) { diff --git a/src/core/devices/nm-device-vxlan.c b/src/core/devices/nm-device-vxlan.c index 9c54791462..4058287c00 100644 --- a/src/core/devices/nm-device-vxlan.c +++ b/src/core/devices/nm-device-vxlan.c @@ -384,8 +384,7 @@ complete_connection(NMDevice *device, NULL, _("VXLAN connection"), NULL, - NULL, - TRUE); + NULL); s_vxlan = nm_connection_get_setting_vxlan(connection); if (!s_vxlan) { diff --git a/src/core/devices/nm-device-wpan.c b/src/core/devices/nm-device-wpan.c index 7885355db7..67f845c868 100644 --- a/src/core/devices/nm-device-wpan.c +++ b/src/core/devices/nm-device-wpan.c @@ -53,8 +53,7 @@ complete_connection(NMDevice *device, NULL, _("WPAN connection"), NULL, - NULL, - TRUE); + NULL); s_wpan = NM_SETTING_WPAN(nm_connection_get_setting(connection, NM_TYPE_SETTING_WPAN)); if (!s_wpan) { diff --git a/src/core/devices/team/nm-device-team.c b/src/core/devices/team/nm-device-team.c index a4c77f7f49..40779c8997 100644 --- a/src/core/devices/team/nm-device-team.c +++ b/src/core/devices/team/nm-device-team.c @@ -130,8 +130,7 @@ complete_connection(NMDevice *device, NULL, _("Team connection"), "team", - NULL, - TRUE); + NULL); _nm_connection_ensure_setting(connection, NM_TYPE_SETTING_TEAM); diff --git a/src/core/devices/wifi/nm-device-iwd-p2p.c b/src/core/devices/wifi/nm-device-iwd-p2p.c index fadc672233..184c8ec583 100644 --- a/src/core/devices/wifi/nm-device-iwd-p2p.c +++ b/src/core/devices/wifi/nm-device-iwd-p2p.c @@ -301,8 +301,7 @@ complete_connection(NMDevice *device, setting_name, setting_name, NULL, - NULL, - TRUE); + NULL); return TRUE; } diff --git a/src/core/devices/wifi/nm-device-iwd.c b/src/core/devices/wifi/nm-device-iwd.c index a7ea14e4e4..fa6e2f9d3a 100644 --- a/src/core/devices/wifi/nm-device-iwd.c +++ b/src/core/devices/wifi/nm-device-iwd.c @@ -1074,8 +1074,7 @@ complete_connection(NMDevice *device, ssid_utf8, ssid_utf8, NULL, - NULL, - TRUE); + NULL); if (hidden) g_object_set(s_wifi, NM_SETTING_WIRELESS_HIDDEN, TRUE, NULL); diff --git a/src/core/devices/wifi/nm-device-olpc-mesh.c b/src/core/devices/wifi/nm-device-olpc-mesh.c index 8e1e779b34..b62dc3115b 100644 --- a/src/core/devices/wifi/nm-device-olpc-mesh.c +++ b/src/core/devices/wifi/nm-device-olpc-mesh.c @@ -111,8 +111,7 @@ complete_connection(NMDevice *device, NULL, _("Mesh"), NULL, - NULL, - FALSE); /* No IPv6 by default */ + NULL); return TRUE; } diff --git a/src/core/devices/wifi/nm-device-wifi-p2p.c b/src/core/devices/wifi/nm-device-wifi-p2p.c index f06383b102..957a2df64f 100644 --- a/src/core/devices/wifi/nm-device-wifi-p2p.c +++ b/src/core/devices/wifi/nm-device-wifi-p2p.c @@ -319,8 +319,7 @@ complete_connection(NMDevice *device, setting_name, setting_name, NULL, - NULL, - TRUE); + NULL); return TRUE; } diff --git a/src/core/devices/wifi/nm-device-wifi.c b/src/core/devices/wifi/nm-device-wifi.c index 7b17f55c2d..06eee14245 100644 --- a/src/core/devices/wifi/nm-device-wifi.c +++ b/src/core/devices/wifi/nm-device-wifi.c @@ -1296,8 +1296,7 @@ complete_connection(NMDevice *device, ssid_utf8, ssid_utf8, NULL, - nm_setting_wireless_get_mac_address(s_wifi) ? NULL : nm_device_get_iface(device), - TRUE); + nm_setting_wireless_get_mac_address(s_wifi) ? NULL : nm_device_get_iface(device)); if (hidden) g_object_set(s_wifi, NM_SETTING_WIRELESS_HIDDEN, TRUE, NULL); diff --git a/src/core/devices/wwan/nm-modem-broadband.c b/src/core/devices/wwan/nm-modem-broadband.c index 8ea16ee75d..018e53066c 100644 --- a/src/core/devices/wwan/nm-modem-broadband.c +++ b/src/core/devices/wwan/nm-modem-broadband.c @@ -917,8 +917,7 @@ complete_connection(NMModem *modem, NULL, _("GSM connection"), NULL, - NULL, - TRUE); + NULL); return TRUE; } @@ -938,8 +937,7 @@ complete_connection(NMModem *modem, NULL, _("CDMA connection"), NULL, - iface, - TRUE); + iface); return TRUE; } diff --git a/src/core/nm-manager.c b/src/core/nm-manager.c index a4a43b37cc..71d8f83cf7 100644 --- a/src/core/nm-manager.c +++ b/src/core/nm-manager.c @@ -6952,8 +6952,7 @@ impl_manager_add_and_activate_connection(NMDBusObject *obj, NULL, _("VPN connection"), NULL, - NULL, - FALSE); /* No IPv6 by default for now */ + NULL); } else { conns = nm_settings_connections_array_to_connections( nm_settings_get_connections(priv->settings, NULL), From cfe6e730b32c4165850d89474b7586dcea6c05ec Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 7 Jan 2025 08:37:25 +0100 Subject: [PATCH 05/28] device: don't log connection UUIDs on device creation It's irrelevant, doesn't look good, and might possibly be not there because the connection has not been normalized yet. --- src/core/devices/nm-device-ipvlan.c | 5 ++--- src/core/devices/nm-device-macvlan.c | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/core/devices/nm-device-ipvlan.c b/src/core/devices/nm-device-ipvlan.c index 6ca20b9d28..00a1b57937 100644 --- a/src/core/devices/nm-device-ipvlan.c +++ b/src/core/devices/nm-device-ipvlan.c @@ -183,9 +183,8 @@ create_and_realize(NMDevice *device, g_set_error(error, NM_DEVICE_ERROR, NM_DEVICE_ERROR_FAILED, - "unsupported IPVLAN mode %u in connection %s", - nm_setting_ipvlan_get_mode(s_ipvlan), - nm_connection_get_uuid(connection)); + "unsupported IPVLAN mode %u", + nm_setting_ipvlan_get_mode(s_ipvlan)); return FALSE; } lnk.mode = setting_mode_to_platform(nm_setting_ipvlan_get_mode(s_ipvlan)); diff --git a/src/core/devices/nm-device-macvlan.c b/src/core/devices/nm-device-macvlan.c index 8300023662..9501e8f15c 100644 --- a/src/core/devices/nm-device-macvlan.c +++ b/src/core/devices/nm-device-macvlan.c @@ -232,9 +232,8 @@ create_and_realize(NMDevice *device, g_set_error(error, NM_DEVICE_ERROR, NM_DEVICE_ERROR_FAILED, - "unsupported MACVLAN mode %u in connection %s", - nm_setting_macvlan_get_mode(s_macvlan), - nm_connection_get_uuid(connection)); + "unsupported MACVLAN mode %u", + nm_setting_macvlan_get_mode(s_macvlan)); return FALSE; } lnk.no_promisc = !nm_setting_macvlan_get_promiscuous(s_macvlan); From 25871f1971ad80d07c8456c42148e9bab9987954 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Mon, 13 Jan 2025 14:15:33 +0100 Subject: [PATCH 06/28] manager: reword some error messages They've been a little too cryptic and unnecessarily long before. --- src/core/devices/nm-device-factory.c | 2 +- src/core/nm-manager.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/devices/nm-device-factory.c b/src/core/devices/nm-device-factory.c index bd771b7cf4..1585836281 100644 --- a/src/core/devices/nm-device-factory.c +++ b/src/core/devices/nm-device-factory.c @@ -147,7 +147,7 @@ nm_device_factory_get_connection_iface(NMDeviceFactory *factory, g_set_error(error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_FAILED, - "failed to determine interface name: error determine name for %s", + "failed to determine interface name for a %s", nm_connection_get_connection_type(connection)); return NULL; } diff --git a/src/core/nm-manager.c b/src/core/nm-manager.c index 71d8f83cf7..e23cf0cabf 100644 --- a/src/core/nm-manager.c +++ b/src/core/nm-manager.c @@ -2522,7 +2522,7 @@ return_ifname_fom_connection: g_set_error(error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_FAILED, - "failed to determine interface name: error determine name for %s", + "failed to determine interface name for a %s", nm_connection_get_connection_type(connection)); } return iface; From 57e140d961604e5d33a4a7a73210b3ece77c6e4b Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 7 Jan 2025 08:33:29 +0100 Subject: [PATCH 07/28] manager: split device creation off from validate_activation_request() Make validate_activation_request() only do the validation -- split the determination of the device into find_device_for_activation(). The point of this is to be able complete the connection and actually create a virtual device after the validation. I believe this is also somewhat easier to follow now that the procedure does what its name says. --- src/core/nm-manager.c | 107 +++++++++++++++++++++++++++--------------- 1 file changed, 70 insertions(+), 37 deletions(-) diff --git a/src/core/nm-manager.c b/src/core/nm-manager.c index e23cf0cabf..8f77165bf6 100644 --- a/src/core/nm-manager.c +++ b/src/core/nm-manager.c @@ -6403,17 +6403,11 @@ nm_manager_activate_connection(NMManager *self, * @sett_conn: the #NMSettingsConnection to be activated, or %NULL if there * is only a partial activation. * @connection: the partial #NMConnection to be activated (if @sett_conn is unspecified) - * @device_path: the object path of the device to be activated, or NULL - * @out_device: on successful return, the #NMDevice to be activated with @connection - * The caller may pass in a device which shortcuts the lookup by path. - * In this case, the passed in device must have the matching @device_path - * already. - * @out_is_vpn: on successful return, %TRUE if @connection is a VPN connection * @error: location to store an error on failure * - * Performs basic validation on an activation request, including ensuring that - * the requestor is a valid Unix process, is not disallowed in @connection - * permissions, and that a device exists that can activate @connection. + * Performs basic permission validation on an activation request: Ensures that + * the requestor is a valid Unix process and is not disallowed in @connection + * permissions. * * Returns: on success, the #NMAuthSubject representing the requestor, or * %NULL on error @@ -6423,13 +6417,8 @@ validate_activation_request(NMManager *self, GDBusMethodInvocation *context, NMSettingsConnection *sett_conn, NMConnection *connection, - const char *device_path, - NMDevice **out_device, - gboolean *out_is_vpn, GError **error) { - NMDevice *device = NULL; - gboolean is_vpn = FALSE; gs_unref_object NMAuthSubject *subject = NULL; nm_assert(!sett_conn || NM_IS_SETTINGS_CONNECTION(sett_conn)); @@ -6437,8 +6426,6 @@ validate_activation_request(NMManager *self, nm_assert(sett_conn || connection); nm_assert(!connection || !sett_conn || connection == nm_settings_connection_get_connection(sett_conn)); - nm_assert(out_device); - nm_assert(out_is_vpn); if (!connection) connection = nm_settings_connection_get_connection(sett_conn); @@ -6459,6 +6446,51 @@ validate_activation_request(NMManager *self, NM_MANAGER_ERROR_PERMISSION_DENIED, error)) return NULL; + return g_steal_pointer(&subject); +} + +/** + * find_device_for_activation: + * @self: the #NMManager + * @sett_conn: the #NMSettingsConnection to be activated, or %NULL if there + * is only a partial activation. + * @connection: the partial #NMConnection to be activated (if @sett_conn is unspecified) + * @device_path: the object path of the device to be activated, or NULL + * @out_device: on successful return, the #NMDevice to be activated with @connection + * The caller may pass in a device which shortcuts the lookup by path. + * In this case, the passed in device must have the matching @device_path + * already. + * @out_is_vpn: on successful return, %TRUE if @connection is a VPN connection + * @error: location to store an error on failure + * + * Looks up a device that can activate @connection, or indicates the + * connection is a VPN connection that does not require a device. + * + * Returns: %TRUE if the device could be find or connection doesn't + * need one, %FALSE otherwise + */ +static gboolean +find_device_for_activation(NMManager *self, + NMSettingsConnection *sett_conn, + NMConnection *connection, + const char *device_path, + NMDevice **out_device, + gboolean *out_is_vpn, + GError **error) +{ + gboolean is_vpn = FALSE; + NMDevice *device = NULL; + + nm_assert(!sett_conn || NM_IS_SETTINGS_CONNECTION(sett_conn)); + nm_assert(!connection || NM_IS_CONNECTION(connection)); + nm_assert(sett_conn || connection); + nm_assert(!connection || !sett_conn + || connection == nm_settings_connection_get_connection(sett_conn)); + nm_assert(out_device); + nm_assert(out_is_vpn); + + if (!connection) + connection = nm_settings_connection_get_connection(sett_conn); is_vpn = _connection_is_vpn(connection); @@ -6475,7 +6507,7 @@ validate_activation_request(NMManager *self, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_DEVICE, "Device not found"); - return NULL; + return FALSE; } } else if (!is_vpn) { gs_free_error GError *local = NULL; @@ -6497,13 +6529,13 @@ validate_activation_request(NMManager *self, NM_MANAGER_ERROR_UNKNOWN_DEVICE, "No suitable device found for this connection (%s).", local->message); - return NULL; + return FALSE; } /* Look for an existing device with the connection's interface name */ iface = nm_manager_get_connection_iface(self, connection, NULL, NULL, error); if (!iface) - return NULL; + return FALSE; device = find_device_by_iface(self, iface, connection, NULL, NULL); if (!device) { @@ -6511,7 +6543,7 @@ validate_activation_request(NMManager *self, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_DEVICE, "Failed to find a compatible device for this connection"); - return NULL; + return FALSE; } } } @@ -6520,7 +6552,8 @@ validate_activation_request(NMManager *self, *out_device = device; *out_is_vpn = is_vpn; - return g_steal_pointer(&subject); + + return TRUE; } /*****************************************************************************/ @@ -6637,17 +6670,14 @@ impl_manager_activate_connection(NMDBusObject *obj, goto error; } - subject = validate_activation_request(self, - invocation, - sett_conn, - NULL, - device_path, - &device, - &is_vpn, - &error); + subject = validate_activation_request(self, invocation, sett_conn, NULL, &error); if (!subject) goto error; + if (!find_device_for_activation(self, sett_conn, NULL, device_path, &device, &is_vpn, &error)) { + goto error; + } + active = _new_active_connection(self, is_vpn, sett_conn, @@ -6920,17 +6950,20 @@ impl_manager_add_and_activate_connection(NMDBusObject *obj, NM_SETTING_PARSE_FLAGS_STRICT, NULL); - subject = validate_activation_request(self, - invocation, - NULL, - incompl_conn, - device_path, - &device, - &is_vpn, - &error); + subject = validate_activation_request(self, invocation, NULL, incompl_conn, &error); if (!subject) goto error; + if (!find_device_for_activation(self, + NULL, + incompl_conn, + device_path, + &device, + &is_vpn, + &error)) { + goto error; + } + if (is_vpn) { /* Try to fill the VPN's connection setting and name at least */ if (!nm_connection_get_setting_vpn(incompl_conn)) { From eb635c23a7f7486c3d7f67a092d5b39ccfad15ca Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 7 Jan 2025 08:27:26 +0100 Subject: [PATCH 08/28] manager: create virtual devices on AddAndActivate() If the connection didn't exist in advance, there's no unrealized device, and find_device_by_iface() is not going to get us one. Call system_create_virtual_device() afrer nm_utils_complete_generic() completes the connection for virtual devices. Make sure we do proper cleanup if we happen to fail the activation later, so that de device doesn't end up hanging there. --- src/core/nm-manager.c | 85 ++++++++++++++++++++++++------------------- 1 file changed, 48 insertions(+), 37 deletions(-) diff --git a/src/core/nm-manager.c b/src/core/nm-manager.c index 8f77165bf6..6ff3b6e85f 100644 --- a/src/core/nm-manager.c +++ b/src/core/nm-manager.c @@ -6538,18 +6538,9 @@ find_device_for_activation(NMManager *self, return FALSE; device = find_device_by_iface(self, iface, connection, NULL, NULL); - if (!device) { - g_set_error_literal(error, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_UNKNOWN_DEVICE, - "Failed to find a compatible device for this connection"); - return FALSE; - } } } - nm_assert(is_vpn || NM_IS_DEVICE(device)); - *out_device = device; *out_is_vpn = is_vpn; @@ -6674,7 +6665,14 @@ impl_manager_activate_connection(NMDBusObject *obj, if (!subject) goto error; - if (!find_device_for_activation(self, sett_conn, NULL, device_path, &device, &is_vpn, &error)) { + if (!find_device_for_activation(self, sett_conn, NULL, device_path, &device, &is_vpn, &error)) + goto error; + + if (!device && !is_vpn) { + g_set_error_literal(&error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_UNKNOWN_DEVICE, + "Failed to find a compatible device for this connection"); goto error; } @@ -6705,6 +6703,10 @@ impl_manager_activate_connection(NMDBusObject *obj, return; error: + if (device && nm_device_is_software(device)) { + if (_check_remove_dev_on_link_deleted(self, device)) + remove_device(self, device, FALSE); + } if (sett_conn) { nm_audit_log_connection_op(NM_AUDIT_OP_CONN_ACTIVATE, sett_conn, @@ -6797,6 +6799,7 @@ _add_and_activate_auth_done(NMManager *self, const char *error_desc) { NMManagerPrivate *priv; + NMDevice *device; GError *error = NULL; if (!success) { @@ -6809,6 +6812,12 @@ _add_and_activate_auth_done(NMManager *self, nm_active_connection_get_subject(active), error->message); g_dbus_method_invocation_take_error(invocation, error); + + device = nm_active_connection_get_device(active); + if (device && nm_device_is_software(device)) { + if (_check_remove_dev_on_link_deleted(self, device)) + remove_device(self, device, FALSE); + } return; } @@ -6964,32 +6973,11 @@ impl_manager_add_and_activate_connection(NMDBusObject *obj, goto error; } - if (is_vpn) { - /* Try to fill the VPN's connection setting and name at least */ - if (!nm_connection_get_setting_vpn(incompl_conn)) { - error = g_error_new_literal(NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_MISSING_SETTING, - "VPN connections require a 'vpn' setting"); - g_prefix_error(&error, "%s: ", NM_SETTING_VPN_SETTING_NAME); - goto error; - } + conns = nm_settings_connections_array_to_connections( + nm_settings_get_connections(priv->settings, NULL), + -1); - conns = nm_settings_connections_array_to_connections( - nm_settings_get_connections(priv->settings, NULL), - -1); - - nm_utils_complete_generic(priv->platform, - incompl_conn, - NM_SETTING_VPN_SETTING_NAME, - conns, - NULL, - _("VPN connection"), - NULL, - NULL); - } else { - conns = nm_settings_connections_array_to_connections( - nm_settings_get_connections(priv->settings, NULL), - -1); + if (device) { /* Let each device subclass complete the connection */ if (!nm_device_complete_connection(device, incompl_conn, @@ -6997,9 +6985,32 @@ impl_manager_add_and_activate_connection(NMDBusObject *obj, conns, &error)) goto error; - } + } else { + nm_utils_complete_generic(priv->platform, + incompl_conn, + nm_connection_get_connection_type(incompl_conn), + conns, + is_vpn ? _("VPN connection") : NULL, + nm_connection_get_connection_type(incompl_conn), + NULL, + NULL); - nm_assert(_nm_connection_verify(incompl_conn, NULL) == NM_SETTING_VERIFY_SUCCESS); + if (!nm_connection_verify(incompl_conn, &error)) + goto error; + + if (!is_vpn && !device) { + nm_assert(nm_connection_is_virtual(incompl_conn)); + + device = system_create_virtual_device(self, incompl_conn); + if (!device) { + g_set_error_literal(&error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_UNKNOWN_DEVICE, + "Failed to create a device for this connection"); + goto error; + } + } + } active = _new_active_connection(self, is_vpn, From 7fc22ef5de5fe6da9c9bc765da7880d8f4514df3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Tue, 19 Nov 2024 09:42:14 +0100 Subject: [PATCH 09/28] nmcs: oci: use macro to log warnings --- src/nm-cloud-setup/nmcs-provider-oci.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/nm-cloud-setup/nmcs-provider-oci.c b/src/nm-cloud-setup/nmcs-provider-oci.c index 324d88bd84..b6ed6b1713 100644 --- a/src/nm-cloud-setup/nmcs-provider-oci.c +++ b/src/nm-cloud-setup/nmcs-provider-oci.c @@ -82,6 +82,8 @@ detect(NMCSProvider *provider, GTask *task) /*****************************************************************************/ +#define _VNIC_WARN(msg) _LOGW("get-config: " msg "(VNIC %s idx=%zu)", vnic_id, i) + static void _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) { @@ -112,14 +114,14 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) for (i = 0; i < json_array_size(vnics); i++) { json_t *vnic, *field; - const char *vnic_id, *val; + const char *vnic_id = "", *val; gs_free char *mac = NULL; in_addr_t addr; int prefix; vnic = json_array_get(vnics, i); if (!json_is_object(vnic)) { - _LOGW("get-config: JSON parse failure for VNIC at index %zu, ignoring VNIC", i); + _VNIC_WARN("JSON parse failure, ignoring VNIC"); continue; } @@ -129,9 +131,7 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) field = json_object_get(vnic, "macAddr"); val = field && json_is_string(field) ? json_string_value(field) : NULL; if (!val) { - _LOGW("get-config: missing or invalid 'macAddr' (VNIC %s idx=%zu), ignoring VNIC", - vnic_id, - i); + _VNIC_WARN("missing or invalid 'macAddr', ignoring VNIC"); continue; } @@ -147,7 +147,7 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) config_iface_data->ipv4s_arr = g_new(in_addr_t, 1); config_iface_data->ipv4s_arr[0] = addr; } else { - _LOGW("get-config: missing or invalid 'privateIp' (VNIC %s idx=%zu)", vnic_id, i); + _VNIC_WARN("missing or invalid 'privateIp'"); } field = json_object_get(vnic, "virtualRouterIp"); @@ -156,7 +156,7 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) config_iface_data->has_gateway = TRUE; config_iface_data->gateway = addr; } else { - _LOGW("get-config: missing or invalid 'virtualRouterIp' (VNIC %s idx=%zu)", vnic_id, i); + _VNIC_WARN("missing or invalid 'virtualRouterIp'"); } field = json_object_get(vnic, "subnetCidrBlock"); @@ -166,7 +166,7 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) config_iface_data->cidr_addr = addr; config_iface_data->cidr_prefix = prefix; } else { - _LOGW("get-config: missing or invalid 'subnetCidrBlock' (VNIC %s idx=%zu)", vnic_id, i); + _VNIC_WARN("missing or invalid 'subnetCidrBlock'"); } } From dd5b4fcf24595bc09086b5ce1f28702806dc5526 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Tue, 19 Nov 2024 10:11:43 +0100 Subject: [PATCH 10/28] nmcs: remove nmcs_provider_*_get_type forward declaration There is no need to avoid including the full header, they are small headers with some GLib type system stuff and no more. Just include them where they are needed. --- src/nm-cloud-setup/nmcs-provider.c | 2 ++ src/nm-cloud-setup/nmcs-provider.h | 7 ------- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/nm-cloud-setup/nmcs-provider.c b/src/nm-cloud-setup/nmcs-provider.c index fe8fedcf1d..0f06c4e221 100644 --- a/src/nm-cloud-setup/nmcs-provider.c +++ b/src/nm-cloud-setup/nmcs-provider.c @@ -3,6 +3,8 @@ #include "libnm-client-aux-extern/nm-default-client.h" #include "nmcs-provider.h" +#include "nmcs-provider-aliyun.h" +#include "nmcs-provider-oci.h" #include "nm-cloud-setup-utils.h" diff --git a/src/nm-cloud-setup/nmcs-provider.h b/src/nm-cloud-setup/nmcs-provider.h index 9e5eeebe69..98e50ea378 100644 --- a/src/nm-cloud-setup/nmcs-provider.h +++ b/src/nm-cloud-setup/nmcs-provider.h @@ -220,11 +220,4 @@ void nmcs_provider_get_config(NMCSProvider *provider, NMCSProviderGetConfigResult * nmcs_provider_get_config_finish(NMCSProvider *provider, GAsyncResult *result, GError **error); -/*****************************************************************************/ - -/* Forward declare the implemented gtype getters so we can use it at a few places without requiring - * to include the full header. The other parts of those headers should not be used aside where they - * are necessary. */ -GType nmcs_provider_aliyun_get_type(void); - #endif /* __NMCS_PROVIDER_H__ */ From cfd7dd86c965d3e89485c1708ba52e0dd3451162 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Tue, 19 Nov 2024 17:04:09 +0100 Subject: [PATCH 11/28] cloud-setup: parse OCI metadata related to VLAN config Baremetal instances in Oracle Cloud require special VLAN config. Parse the metadata related to it. --- src/nm-cloud-setup/nmcs-provider-oci.c | 75 ++++++++++++++++++++++++-- src/nm-cloud-setup/nmcs-provider.c | 9 ++++ src/nm-cloud-setup/nmcs-provider.h | 6 +++ 3 files changed, 87 insertions(+), 3 deletions(-) diff --git a/src/nm-cloud-setup/nmcs-provider-oci.c b/src/nm-cloud-setup/nmcs-provider-oci.c index b6ed6b1713..330aeff7ee 100644 --- a/src/nm-cloud-setup/nmcs-provider-oci.c +++ b/src/nm-cloud-setup/nmcs-provider-oci.c @@ -13,6 +13,8 @@ #define NM_OCI_HOST "169.254.169.254" #define NM_OCI_BASE "http://" NM_OCI_HOST +#define MAX_NIC_INDEX 1 + NMCS_DEFINE_HOST_BASE(_oci_base, NMCS_ENV_NM_CLOUD_SETUP_OCI_HOST, NM_OCI_BASE); #define _oci_uri_concat(...) nmcs_utils_uri_build_concat(_oci_base(), "opc/v2/", __VA_ARGS__) @@ -92,6 +94,9 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) gs_unref_bytes GBytes *response = NULL; gs_free_error GError *error = NULL; nm_auto_decref_json json_t *vnics = NULL; + gboolean is_baremetal; + const char *phys_nic_macs[MAX_NIC_INDEX + 1] = {NULL}; + GHashTableIter h_iter; size_t i; nm_http_client_poll_req_finish(NM_HTTP_CLIENT(source), result, NULL, &response, &error); @@ -112,12 +117,21 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) goto out; } + if (json_array_size(vnics) > 0) { + is_baremetal = NULL != json_object_get(json_array_get(vnics, 0), "nicIndex"); + _LOGI("get-config: detected %s instance", is_baremetal ? "baremetal" : "VM"); + } else { + is_baremetal = FALSE; + _LOGI("get-config: empty VNICs metadata, cannot detect instance type"); + } + for (i = 0; i < json_array_size(vnics); i++) { json_t *vnic, *field; const char *vnic_id = "", *val; - gs_free char *mac = NULL; + gs_free char *mac = NULL; in_addr_t addr; int prefix; + json_int_t nic_index = -1, vlan_tag = -1; vnic = json_array_get(vnics, i); if (!json_is_object(vnic)) { @@ -130,12 +144,28 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) field = json_object_get(vnic, "macAddr"); val = field && json_is_string(field) ? json_string_value(field) : NULL; - if (!val) { + mac = val ? nmcs_utils_hwaddr_normalize(val, json_string_length(field)) : NULL; + if (!mac) { _VNIC_WARN("missing or invalid 'macAddr', ignoring VNIC"); continue; } - mac = nmcs_utils_hwaddr_normalize(val, json_string_length(field)); + if (is_baremetal) { + field = json_object_get(vnic, "nicIndex"); + nic_index = field && json_is_integer(field) ? json_integer_value(field) : -1; + if (nic_index < 0 || nic_index > MAX_NIC_INDEX) { + _VNIC_WARN("missing or invalid 'nicIndex', ignoring VNIC"); + continue; + } + + field = json_object_get(vnic, "vlanTag"); + vlan_tag = field && json_is_integer(field) ? json_integer_value(field) : -1; + if (vlan_tag < 0) { + _VNIC_WARN("missing or invalid 'vlanTag', ignoring VNIC"); + continue; + } + } + config_iface_data = nmcs_provider_get_config_iface_data_create(get_config_data, FALSE, mac); config_iface_data->iface_idx = i; @@ -168,6 +198,45 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) } else { _VNIC_WARN("missing or invalid 'subnetCidrBlock'"); } + + if (is_baremetal) { + gboolean is_phys_nic = vlan_tag == 0; + + /* In baremetal instances, configure VNICs' VLAN (physical NICs don't need it) */ + if (is_phys_nic) { + config_iface_data->priv.oci.vlan_tag = 0; + config_iface_data->priv.oci.parent_hwaddr = NULL; + phys_nic_macs[nic_index] = config_iface_data->hwaddr; + } else { + /* We might not have all the physical NICs' MACs yet, save nicIndex for later */ + config_iface_data->priv.oci.parent_hwaddr = GINT_TO_POINTER((int) nic_index); + config_iface_data->priv.oci.vlan_tag = vlan_tag; + } + } + } + + if (is_baremetal) { + g_hash_table_iter_init(&h_iter, get_config_data->result_dict); + + /* Now that all the metadata is processed we should have all the physical NICs' MACs */ + while (g_hash_table_iter_next(&h_iter, NULL, (gpointer *) &config_iface_data)) { + bool is_phys_nic = config_iface_data->priv.oci.vlan_tag == 0; + int nic_index = GPOINTER_TO_INT(config_iface_data->priv.oci.parent_hwaddr); + + if (is_phys_nic) + continue; + + if (phys_nic_macs[nic_index] == NULL) { + _LOGW("get-config: physical NIC for nicIndex=%d not found, ignoring VNIC " + "(VNIC macAddr=%s)", + nic_index, + config_iface_data->hwaddr); + g_hash_table_iter_remove(&h_iter); + continue; + } + + config_iface_data->priv.oci.parent_hwaddr = g_strdup(phys_nic_macs[nic_index]); + } } out: diff --git a/src/nm-cloud-setup/nmcs-provider.c b/src/nm-cloud-setup/nmcs-provider.c index 0f06c4e221..251749ba79 100644 --- a/src/nm-cloud-setup/nmcs-provider.c +++ b/src/nm-cloud-setup/nmcs-provider.c @@ -188,6 +188,7 @@ nmcs_provider_get_config_iface_data_create(NMCSProviderGetConfigTaskData *get_co iface_data = g_slice_new(NMCSProviderGetConfigIfaceData); *iface_data = (NMCSProviderGetConfigIfaceData) { + .provider = g_object_ref(get_config_data->self), .get_config_data = get_config_data, .hwaddr = g_strdup(hwaddr), .iface_idx = -1, @@ -203,6 +204,11 @@ nmcs_provider_get_config_iface_data_create(NMCSProviderGetConfigTaskData *get_co iface_data->priv.aliyun = (typeof(iface_data->priv.aliyun)) { .has_primary_ip_address = FALSE, }; + } else if (G_OBJECT_TYPE(get_config_data->self) == nmcs_provider_oci_get_type()) { + iface_data->priv.oci = (typeof(iface_data->priv.oci)) { + .vlan_tag = 0, + .parent_hwaddr = NULL, + }; } /* the has does not own the key (iface_datta->hwaddr), the lifetime of the @@ -220,6 +226,9 @@ _iface_data_free(gpointer data) g_free(iface_data->ipv4s_arr); nm_g_ptr_array_unref(iface_data->iproutes); g_free((char *) iface_data->hwaddr); + if (G_OBJECT_TYPE(iface_data->provider) == nmcs_provider_oci_get_type()) + g_free((char *) iface_data->priv.oci.parent_hwaddr); + g_clear_object(&iface_data->provider); nm_g_slice_free(iface_data); } diff --git a/src/nm-cloud-setup/nmcs-provider.h b/src/nm-cloud-setup/nmcs-provider.h index 98e50ea378..95a591c651 100644 --- a/src/nm-cloud-setup/nmcs-provider.h +++ b/src/nm-cloud-setup/nmcs-provider.h @@ -17,6 +17,8 @@ typedef struct { * dictionary. */ const char *hwaddr; + struct _NMCSProvider *provider; + struct _NMCSProviderGetConfigTaskData *get_config_data; in_addr_t *ipv4s_arr; @@ -51,6 +53,10 @@ typedef struct { bool has_primary_ip_address : 1; bool ipv4s_arr_ordered : 1; } aliyun; + struct { + guint32 vlan_tag; /* 0 if no VLAN is needed */ + const char *parent_hwaddr; + } oci; } priv; } NMCSProviderGetConfigIfaceData; From 98f8224376a05f48e3ba03a23e9eb29d2567c029 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Mon, 25 Nov 2024 12:04:35 +0100 Subject: [PATCH 12/28] cloud-setup: oci: remove the max 2 phys NICs limit Right now, on any baremetal only max. 2 physical NICs are available. This might change in the future, so better to directly accept larger nicIndex if we receive it. No behaviour change with this, just remove an artificial limit. --- src/nm-cloud-setup/nmcs-provider-oci.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/nm-cloud-setup/nmcs-provider-oci.c b/src/nm-cloud-setup/nmcs-provider-oci.c index 330aeff7ee..17cd997ef8 100644 --- a/src/nm-cloud-setup/nmcs-provider-oci.c +++ b/src/nm-cloud-setup/nmcs-provider-oci.c @@ -13,8 +13,6 @@ #define NM_OCI_HOST "169.254.169.254" #define NM_OCI_BASE "http://" NM_OCI_HOST -#define MAX_NIC_INDEX 1 - NMCS_DEFINE_HOST_BASE(_oci_base, NMCS_ENV_NM_CLOUD_SETUP_OCI_HOST, NM_OCI_BASE); #define _oci_uri_concat(...) nmcs_utils_uri_build_concat(_oci_base(), "opc/v2/", __VA_ARGS__) @@ -95,7 +93,7 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) gs_free_error GError *error = NULL; nm_auto_decref_json json_t *vnics = NULL; gboolean is_baremetal; - const char *phys_nic_macs[MAX_NIC_INDEX + 1] = {NULL}; + gs_unref_ptrarray GPtrArray *phys_nic_macs = NULL; GHashTableIter h_iter; size_t i; @@ -125,6 +123,9 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) _LOGI("get-config: empty VNICs metadata, cannot detect instance type"); } + if (is_baremetal) + phys_nic_macs = g_ptr_array_sized_new(16); + for (i = 0; i < json_array_size(vnics); i++) { json_t *vnic, *field; const char *vnic_id = "", *val; @@ -153,7 +154,7 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) if (is_baremetal) { field = json_object_get(vnic, "nicIndex"); nic_index = field && json_is_integer(field) ? json_integer_value(field) : -1; - if (nic_index < 0 || nic_index > MAX_NIC_INDEX) { + if (nic_index < 0 || nic_index >= 1024) { /* 1024 = random limit to prevent abuse*/ _VNIC_WARN("missing or invalid 'nicIndex', ignoring VNIC"); continue; } @@ -206,7 +207,10 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) if (is_phys_nic) { config_iface_data->priv.oci.vlan_tag = 0; config_iface_data->priv.oci.parent_hwaddr = NULL; - phys_nic_macs[nic_index] = config_iface_data->hwaddr; + if (nic_index >= phys_nic_macs->len) + g_ptr_array_set_size(phys_nic_macs, + NM_MAX((guint) (nic_index + 1), phys_nic_macs->len * 2)); + phys_nic_macs->pdata[nic_index] = (gpointer) config_iface_data->hwaddr; } else { /* We might not have all the physical NICs' MACs yet, save nicIndex for later */ config_iface_data->priv.oci.parent_hwaddr = GINT_TO_POINTER((int) nic_index); @@ -226,7 +230,7 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) if (is_phys_nic) continue; - if (phys_nic_macs[nic_index] == NULL) { + if (nic_index >= phys_nic_macs->len || phys_nic_macs->pdata[nic_index] == NULL) { _LOGW("get-config: physical NIC for nicIndex=%d not found, ignoring VNIC " "(VNIC macAddr=%s)", nic_index, @@ -235,7 +239,7 @@ _get_config_done_cb(GObject *source, GAsyncResult *result, gpointer user_data) continue; } - config_iface_data->priv.oci.parent_hwaddr = g_strdup(phys_nic_macs[nic_index]); + config_iface_data->priv.oci.parent_hwaddr = g_strdup(phys_nic_macs->pdata[nic_index]); } } From f7c8597835d35da03099a4589567dff243ca1314 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 13 Dec 2024 15:11:43 +0100 Subject: [PATCH 13/28] test/cloud-meta-mock: do not print what we listen on if we got a FD This message is useless for non-interactive use and clobbers over otherwise very appealing test output. The callers knows what we're going to listen on, it passed us the file descriptor. --- tools/test-cloud-meta-mock.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tools/test-cloud-meta-mock.py b/tools/test-cloud-meta-mock.py index a396e0da41..f7f48bc92f 100755 --- a/tools/test-cloud-meta-mock.py +++ b/tools/test-cloud-meta-mock.py @@ -276,7 +276,10 @@ httpd = SocketHTTPServer( allow_default=allow_default, ) -print("Listening on http://%s:%d" % (httpd.server_address[0], httpd.server_address[1])) +if fileno is None: + print( + "Listening on http://%s:%d" % (httpd.server_address[0], httpd.server_address[1]) + ) httpd.server_activate() httpd.serve_forever() From 392f76a23b02c328da11b513ec89fc24ae979dfd Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 19 Nov 2024 12:15:58 +0100 Subject: [PATCH 14/28] test/nm-service: add Device.HwAddress property nm-cloud-setup finds devices by hwaddr. Let's expose it for all device types, so that we're be able to test once we add VLANs and MACVLANs. --- tools/test-networkmanager-service.py | 34 ++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/tools/test-networkmanager-service.py b/tools/test-networkmanager-service.py index b42687219f..16aac4abcb 100755 --- a/tools/test-networkmanager-service.py +++ b/tools/test-networkmanager-service.py @@ -845,13 +845,14 @@ PRP_DEVICE_DEVICE_TYPE = "DeviceType" PRP_DEVICE_AVAILABLE_CONNECTIONS = "AvailableConnections" PRP_DEVICE_LLDP_NEIGHBORS = "LldpNeighbors" PRP_DEVICE_INTERFACE_FLAGS = "InterfaceFlags" +PRP_DEVICE_HW_ADDRESS = "HwAddress" class Device(ExportedObj): path_counter_next = 1 path_prefix = "/org/freedesktop/NetworkManager/Devices/" - def __init__(self, iface, devtype, ident=None): + def __init__(self, iface, devtype, ident=None, hwaddr=None): if ident is None: ident = iface @@ -863,6 +864,7 @@ class Device(ExportedObj): self.dhcp4_config = None self.dhcp6_config = None self.activation_state_change_delay_ms = 50 + self.hwaddr = hwaddr is None if "" else hwaddr self.prp_state = NM.DeviceState.UNAVAILABLE @@ -890,6 +892,7 @@ class Device(ExportedObj): PRP_DEVICE_DEVICE_TYPE: dbus.UInt32(devtype), PRP_DEVICE_AVAILABLE_CONNECTIONS: ExportedObj.to_path_array([]), PRP_DEVICE_INTERFACE_FLAGS: dbus.UInt32(3), # up,lower-up + PRP_DEVICE_HW_ADDRESS: dbus.String(self.hwaddr), PRP_DEVICE_LLDP_NEIGHBORS: dbus.Array( [ dbus.Dictionary( @@ -1166,10 +1169,11 @@ PRP_WIRED_S390_SUBCHANNELS = "S390Subchannels" class WiredDevice(Device): def __init__(self, iface, mac=None, subchannels=None, ident=None): - Device.__init__(self, iface, NM.DeviceType.ETHERNET, ident) - if mac is None: - mac = Util.random_mac(self.ident) + mac = Util.random_mac(iface if ident is None else ident) + + Device.__init__(self, iface, NM.DeviceType.ETHERNET, ident, hwaddr=mac) + if subchannels is None: subchannels = dbus.Array(signature="s") @@ -1212,8 +1216,8 @@ PRP_VLAN_VLAN_ID = "VlanId" class VlanDevice(Device): - def __init__(self, iface, ident=None): - Device.__init__(self, iface, NM.DeviceType.VLAN, ident) + def __init__(self, iface, mac=None, ident=None): + Device.__init__(self, iface, NM.DeviceType.VLAN, ident, hwaddr=mac) props = { PRP_VLAN_HW_ADDRESS: Util.random_mac(self.ident), @@ -1313,10 +1317,10 @@ PRP_WIFI_LAST_SCAN = "LastScan" class WifiDevice(Device): def __init__(self, iface, mac=None, ident=None): - Device.__init__(self, iface, NM.DeviceType.WIFI, ident) - if mac is None: - mac = Util.random_mac(self.ident) + mac = Util.random_mac(iface if ident is None else ident) + + Device.__init__(self, iface, NM.DeviceType.WIFI, ident, hwaddr=mac) self.aps = [] self.scan_cb_id = None @@ -1807,6 +1811,7 @@ class NetworkManager(ExportedObj): iface=_DEFAULT_ARG, ip_iface=_DEFAULT_ARG, dev_type=_DEFAULT_ARG, + hwaddr=_DEFAULT_ARG, ): r = None for d in self.devices: @@ -1826,6 +1831,9 @@ class NetworkManager(ExportedObj): if dev_type is not _DEFAULT_ARG: if not isinstance(d, dev_type): continue + if hwaddr is not _DEFAULT_ARG: + if d.hwaddr.lower() != hwaddr.lower(): + continue yield d def find_device_first( @@ -1835,11 +1843,17 @@ class NetworkManager(ExportedObj): iface=_DEFAULT_ARG, ip_iface=_DEFAULT_ARG, dev_type=_DEFAULT_ARG, + hwaddr=_DEFAULT_ARG, require=None, ): r = None for d in self.find_devices( - ident=ident, path=path, iface=iface, ip_iface=ip_iface, dev_type=dev_type + ident=ident, + path=path, + iface=iface, + ip_iface=ip_iface, + dev_type=dev_type, + hwaddr=hwaddr, ): r = d break From 93983155bfc1ecf4ac98058ef09d255b3d2b3a30 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 3 Dec 2024 10:34:32 +0100 Subject: [PATCH 15/28] test/nm-service: add MacvlanDevice class We'll need to test the nm-cloud-setup OCI multiple VNIC support. --- tools/test-networkmanager-service.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tools/test-networkmanager-service.py b/tools/test-networkmanager-service.py index 16aac4abcb..1504dfdfd0 100755 --- a/tools/test-networkmanager-service.py +++ b/tools/test-networkmanager-service.py @@ -429,6 +429,7 @@ IFACE_AGENT = "org.freedesktop.NetworkManager.SecretAgent" IFACE_WIRED = "org.freedesktop.NetworkManager.Device.Wired" IFACE_MODEM = "org.freedesktop.NetworkManager.Device.Modem" IFACE_VLAN = "org.freedesktop.NetworkManager.Device.Vlan" +IFACE_MACVLAN = "org.freedesktop.NetworkManager.Device.Macvlan" IFACE_WIFI_AP = "org.freedesktop.NetworkManager.AccessPoint" IFACE_ACTIVE_CONNECTION = "org.freedesktop.NetworkManager.Connection.Active" IFACE_VPN_CONNECTION = "org.freedesktop.NetworkManager.VPN.Connection" @@ -611,6 +612,7 @@ class NmUtil: t = s_con[NM.SETTING_CONNECTION_TYPE] if t not in [ NM.SETTING_GSM_SETTING_NAME, + NM.SETTING_MACVLAN_SETTING_NAME, NM.SETTING_VLAN_SETTING_NAME, NM.SETTING_VPN_SETTING_NAME, NM.SETTING_WIMAX_SETTING_NAME, @@ -1208,6 +1210,15 @@ class ModemDevice(Device): self.dbus_interface_add(IFACE_MODEM, props) +############################################################################### + + +class MacvlanDevice(Device): + def __init__(self, iface, mac=None): + Device.__init__(self, iface, NM.DeviceType.MACVLAN, hwaddr=mac) + self.dbus_interface_add(IFACE_MACVLAN, {}) + + ############################################################################### PRP_VLAN_HW_ADDRESS = "HwAddress" From 158adac3a6b5b196d9a23a571301761f4952cd62 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 19 Nov 2024 11:43:18 +0100 Subject: [PATCH 16/28] test/nm-service: create software devices on AddAndActivate() This will be useful for testing VLAN bringup. --- tools/test-networkmanager-service.py | 85 ++++++++++++++++++---------- 1 file changed, 54 insertions(+), 31 deletions(-) diff --git a/tools/test-networkmanager-service.py b/tools/test-networkmanager-service.py index 1504dfdfd0..7c1146fefa 100755 --- a/tools/test-networkmanager-service.py +++ b/tools/test-networkmanager-service.py @@ -1666,30 +1666,16 @@ class NetworkManager(ExportedObj): raise BusErr.UnknownConnectionException("Connection not found") con_hash = con_inst.con_hash - con_type = NmUtil.con_hash_get_type(con_hash) - device = self.find_device_first(path=devpath) - if not device: - if con_type == NM.SETTING_WIRED_SETTING_NAME: - device = self.find_device_first(dev_type=WiredDevice) - elif con_type == NM.SETTING_WIRELESS_SETTING_NAME: - device = self.find_device_first(dev_type=WifiDevice) - elif con_type == NM.SETTING_VLAN_SETTING_NAME: - ifname = con_hash[NM.SETTING_CONNECTION_SETTING_NAME]["interface-name"] - device = VlanDevice(ifname) + device = self.find_device(devpath, con_hash) + if device is None: + device = self.create_device(con_hash) + if device: + # Just created the device, it can start activating right away + device.activation_state_change_delay_ms = 0 self.add_device(device) - elif con_type == NM.SETTING_VPN_SETTING_NAME: - for ac in self.active_connections: - if ac.is_vpn: - continue - if ac.device: - device = ac.device - break - - if not device: - raise BusErr.UnknownDeviceException( - "No device found for the requested iface." - ) + if device is None: + raise BusErr.UnknownDeviceException("Device not found") # See if we need secrets. For the moment, we only support WPA if "802-11-wireless-security" in con_hash: @@ -1748,11 +1734,16 @@ class NetworkManager(ExportedObj): out_signature="ooa{sv}", ) def AddAndActivateConnection2(self, con_hash, devpath, specific_object, options): - device = self.find_device_first( - path=devpath, require=BusErr.UnknownDeviceException - ) - conpath = gl.settings.AddConnection(con_hash) - return (conpath, self.ActivateConnection(conpath, devpath, specific_object), []) + conpath = gl.settings.add_connection(con_hash) + try: + return ( + conpath, + self.ActivateConnection(conpath, devpath, specific_object), + [], + ) + except: + gl.settings.delete_connection(conpath) + raise @dbus.service.method(dbus_interface=IFACE_NM, in_signature="o", out_signature="") def DeactivateConnection(self, active_connection): @@ -1874,6 +1865,37 @@ class NetworkManager(ExportedObj): raise BusErr.UnknownDeviceException("Device not found") return r + def find_device(self, devpath, con_hash): + device = self.find_device_first(path=devpath) + if device: + return device + + con_type = NmUtil.con_hash_get_type(con_hash) + + if con_type == NM.SETTING_WIRED_SETTING_NAME: + return self.find_device_first(dev_type=WiredDevice) + + if con_type == NM.SETTING_WIRELESS_SETTING_NAME: + return self.find_device_first(dev_type=WifiDevice) + + if con_type == NM.SETTING_VPN_SETTING_NAME: + for ac in self.active_connections: + if ac.is_vpn: + continue + if ac.device: + return ac.device + + return None + + def create_device(self, con_hash): + con_type = NmUtil.con_hash_get_type(con_hash) + + if con_type == NM.SETTING_VLAN_SETTING_NAME: + ifname = con_hash[NM.SETTING_CONNECTION_SETTING_NAME]["interface-name"] + return VlanDevice(ifname) + + return None + def add_device(self, device): if self.find_device_first(ident=device.ident, path=device.path) is not None: raise TestError( @@ -2160,7 +2182,7 @@ class Connection(ExportedObj): dbus_interface=IFACE_CONNECTION, in_signature="", out_signature="" ) def Delete(self): - gl.settings.delete_connection(self) + gl.settings.delete_connection(self.path) @dbus.service.method( dbus_interface=IFACE_CONNECTION, in_signature="a{sa{sv}}", out_signature="" @@ -2292,7 +2314,7 @@ class Settings(ExportedObj): def cb(): if hasattr(con_inst, "_remove_next_connection_cb"): del con_inst._remove_next_connection_cb - self.delete_connection(con_inst) + self.delete_connection(con_inst.path) return False # We will delete the connection right away on an idle handler. However, @@ -2309,8 +2331,9 @@ class Settings(ExportedObj): raise BusErr.UnknownConnectionException("Connection not found") self.connections[path].update_connection(con_hash, do_verify_strict) - def delete_connection(self, con_inst): - del self.connections[con_inst.path] + def delete_connection(self, path): + con_inst = self.get_connection(path) + del self.connections[path] self._dbus_property_set( IFACE_SETTINGS, PRP_SETTINGS_CONNECTIONS, From 91524b841999ef0dae37884ffd03359e2fd82c77 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 13 Dec 2024 15:06:08 +0100 Subject: [PATCH 17/28] test/nm-service: create Vlan devices matching the parent by hwaddr This is how OCI VLANS will be looking up their parents. Make sure the mock is able to deal with this. --- tools/test-networkmanager-service.py | 61 +++++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 2 deletions(-) diff --git a/tools/test-networkmanager-service.py b/tools/test-networkmanager-service.py index 7c1146fefa..f87390f1b4 100755 --- a/tools/test-networkmanager-service.py +++ b/tools/test-networkmanager-service.py @@ -1891,8 +1891,65 @@ class NetworkManager(ExportedObj): con_type = NmUtil.con_hash_get_type(con_hash) if con_type == NM.SETTING_VLAN_SETTING_NAME: - ifname = con_hash[NM.SETTING_CONNECTION_SETTING_NAME]["interface-name"] - return VlanDevice(ifname) + iface = con_hash[NM.SETTING_CONNECTION_SETTING_NAME].get("interface-name") + parent_iface = con_hash[NM.SETTING_VLAN_SETTING_NAME].get("parent") + + if NM.SETTING_WIRED_SETTING_NAME in con_hash: + mac = con_hash[NM.SETTING_WIRED_SETTING_NAME].get("mac-address") + else: + mac = None + if mac is not None: + parent_hwaddr = "%02X:%02X:%02X:%02X:%02X:%02X" % ( + mac[0], + mac[1], + mac[2], + mac[3], + mac[4], + mac[5], + ) + else: + parent_hwaddr = _DEFAULT_ARG + + parent_ident = parent_iface if parent_iface is not None else _DEFAULT_ARG + parent_device = self.find_device_first( + dev_type=WiredDevice, hwaddr=parent_hwaddr, ident=parent_ident + ) + if parent_device is None: + parent_device = self.find_device_first( + dev_type=MacvlanDevice, hwaddr=parent_hwaddr, ident=parent_ident + ) + if parent_device is None: + raise BusErr.UnknownDeviceException("Parent device not found") + + if parent_hwaddr is None: + parent_hwaddr = parent_device.hwaddr + + if NM.SETTING_WIRED_SETTING_NAME in con_hash: + mac = con_hash[NM.SETTING_WIRED_SETTING_NAME].get("cloned-mac-address") + else: + mac = None + if mac is not None: + hwaddr = "%02X:%02X:%02X:%02X:%02X:%02X" % ( + mac[0], + mac[1], + mac[2], + mac[3], + mac[4], + mac[5], + ) + else: + hwaddr = None + + if parent_iface is None: + parent_iface = parent_device.ident + + if iface is None: + iface = "%s.%d" % ( + parent_iface, + con_hash[NM.SETTING_VLAN_SETTING_NAME]["id"], + ) + + return VlanDevice(iface, mac=hwaddr) return None From fadfb7bba181b45dea0e7008aed55562bc1dfda3 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 3 Dec 2024 10:35:02 +0100 Subject: [PATCH 18/28] test/nm-service: add support for creating a MACVLAN ...via AddAndActivate. nm-cloud-setup will do that. --- tools/test-networkmanager-service.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tools/test-networkmanager-service.py b/tools/test-networkmanager-service.py index f87390f1b4..8a42633204 100755 --- a/tools/test-networkmanager-service.py +++ b/tools/test-networkmanager-service.py @@ -1951,6 +1951,19 @@ class NetworkManager(ExportedObj): return VlanDevice(iface, mac=hwaddr) + if con_type == NM.SETTING_MACVLAN_SETTING_NAME: + ifname = con_hash[NM.SETTING_CONNECTION_SETTING_NAME]["interface-name"] + mac = con_hash[NM.SETTING_WIRED_SETTING_NAME]["cloned-mac-address"] + hwaddr = "%02X:%02X:%02X:%02X:%02X:%02X" % ( + mac[0], + mac[1], + mac[2], + mac[3], + mac[4], + mac[5], + ) + return MacvlanDevice(ifname, hwaddr) + return None def add_device(self, device): From a0c14665a3733345982250814a2bbfaf684310e0 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 13 Dec 2024 15:35:50 +0100 Subject: [PATCH 19/28] client/test: move run_post() into TestNmcli It attempts to modify attributes clearly belong to TestNmcli such as _skip_test_for_l10n_diff or call methods that are in unittest.TestCase: ====================================================================== ERROR: test_002 (__main__.TestNmcli.test_002) ---------------------------------------------------------------------- Traceback (most recent call last): File ".../src/tests/client/test-client.py", line 1508, in f self.ctx.run_post() ~~~~~~~~~~~~~~~~~^^ File ".../src/tests/client/test-client.py", line 1185, in run_post self.fail( ^^^^^^^^^ AttributeError: 'NMTestContext' object has no attribute 'fail' It has presumably been moved out of TestNmcli at some point, but that seems to have been in error, as it's also pretty specific to the nmcli test cases. Not useful for cloud-init tests that also utilize NMTestContext. Move it back. --- src/tests/client/test-client.py | 221 ++++++++++++++++---------------- 1 file changed, 111 insertions(+), 110 deletions(-) diff --git a/src/tests/client/test-client.py b/src/tests/client/test-client.py index df62751bc0..8b4322e810 100755 --- a/src/tests/client/test-client.py +++ b/src/tests/client/test-client.py @@ -1125,113 +1125,6 @@ class NMTestContext: def async_append_job(self, async_job): self._async_jobs.append(async_job) - def run_post(self): - self.async_wait() - - self.srv_shutdown() - - self._calling_num = None - - results = self.ctx_results - self.ctx_results = None - - if len(results) == 0: - return - - skip_test_for_l10n_diff = self._skip_test_for_l10n_diff - self._skip_test_for_l10n_diff = None - - filename = os.path.abspath( - PathConfiguration.srcdir() - + "/test-client.check-on-disk/" - + self.testMethodName - + ".expected" - ) - - regenerate = conf.get(ENV_NM_TEST_REGENERATE) - - content_expect, results_expect = Util.file_read_expected(filename) - - if results_expect is None: - if not regenerate: - self.fail( - "Failed to parse expected file '%s'. Let the test write the file by rerunning with NM_TEST_REGENERATE=1" - % (filename) - ) - else: - for i in range(0, min(len(results_expect), len(results))): - n = results[i] - if results_expect[i] == n["content"]: - continue - if regenerate: - continue - if n["ignore_l10n_diff"]: - skip_test_for_l10n_diff.append(n["test_name"]) - continue - print( - "\n\n\nThe file '%s' does not have the expected content:" - % (filename) - ) - print("ACTUAL OUTPUT:\n[[%s]]\n" % (n["content"])) - print("EXPECT OUTPUT:\n[[%s]]\n" % (results_expect[i])) - print( - "Let the test write the file by rerunning with NM_TEST_REGENERATE=1" - ) - print( - "See howto in %s for details.\n" - % (PathConfiguration.canonical_script_filename()) - ) - sys.stdout.flush() - self.fail( - "Unexpected output of command, expected %s. Rerun test with NM_TEST_REGENERATE=1 to regenerate files" - % (filename) - ) - if len(results_expect) != len(results): - if not regenerate: - print( - "\n\n\nThe number of tests in %s does not match the expected content (%s vs %s):" - % (filename, len(results_expect), len(results)) - ) - if len(results_expect) < len(results): - print( - "ACTUAL OUTPUT:\n[[%s]]\n" - % (results[len(results_expect)]["content"]) - ) - else: - print( - "EXPECT OUTPUT:\n[[%s]]\n" % (results_expect[len(results)]) - ) - print( - "Let the test write the file by rerunning with NM_TEST_REGENERATE=1" - ) - print( - "See howto in %s for details.\n" - % (PathConfiguration.canonical_script_filename()) - ) - sys.stdout.flush() - self.fail( - "Unexpected output of command, expected %s. Rerun test with NM_TEST_REGENERATE=1 to regenerate files" - % (filename) - ) - - if regenerate: - content_new = b"".join([r["content"] for r in results]) - if content_new != content_expect: - try: - with open(filename, "wb") as content_file: - content_file.write(content_new) - except Exception as e: - self.fail("Failure to write '%s': %s" % (filename, e)) - - if skip_test_for_l10n_diff: - # nmcli loads translations from the installation path. This failure commonly - # happens because you did not install the binary in the --prefix, before - # running the test. Hence, translations are not available or differ. - raise unittest.SkipTest( - "Skipped asserting for localized tests %s. Set NM_TEST_CLIENT_CHECK_L10N=1 to force fail." - % (",".join(skip_test_for_l10n_diff)) - ) - ############################################################################### @@ -1241,6 +1134,7 @@ class TestNmcli(unittest.TestCase): Util.skip_without_dbus_session() Util.skip_without_NM() self.ctx = NMTestContext(self._testMethodName) + self._skip_test_for_l10n_diff = [] def call_nmcli_l( self, @@ -1501,18 +1395,124 @@ class TestNmcli(unittest.TestCase): self.ctx.async_start(wait_all=sync_barrier) + def run_post(self): + self.ctx.async_wait() + self.ctx.srv_shutdown() + + self.ctx._calling_num = None + + results = self.ctx.ctx_results + self.ctx.ctx_results = None + + if len(results) == 0: + return + + skip_test_for_l10n_diff = self._skip_test_for_l10n_diff + self._skip_test_for_l10n_diff = None + + filename = os.path.abspath( + PathConfiguration.srcdir() + + "/test-client.check-on-disk/" + + self._testMethodName + + ".expected" + ) + + regenerate = conf.get(ENV_NM_TEST_REGENERATE) + + content_expect, results_expect = Util.file_read_expected(filename) + + if results_expect is None: + if not regenerate: + self.fail( + "Failed to parse expected file '%s'. Let the test write the file by rerunning with NM_TEST_REGENERATE=1" + % (filename) + ) + else: + for i in range(0, min(len(results_expect), len(results))): + n = results[i] + if results_expect[i] == n["content"]: + continue + if regenerate: + continue + if n["ignore_l10n_diff"]: + skip_test_for_l10n_diff.append(n["test_name"]) + continue + print( + "\n\n\nThe file '%s' does not have the expected content:" + % (filename) + ) + print("ACTUAL OUTPUT:\n[[%s]]\n" % (n["content"])) + print("EXPECT OUTPUT:\n[[%s]]\n" % (results_expect[i])) + print( + "Let the test write the file by rerunning with NM_TEST_REGENERATE=1" + ) + print( + "See howto in %s for details.\n" + % (PathConfiguration.canonical_script_filename()) + ) + sys.stdout.flush() + self.fail( + "Unexpected output of command, expected %s. Rerun test with NM_TEST_REGENERATE=1 to regenerate files" + % (filename) + ) + if len(results_expect) != len(results): + if not regenerate: + print( + "\n\n\nThe number of tests in %s does not match the expected content (%s vs %s):" + % (filename, len(results_expect), len(results)) + ) + if len(results_expect) < len(results): + print( + "ACTUAL OUTPUT:\n[[%s]]\n" + % (results[len(results_expect)]["content"]) + ) + else: + print( + "EXPECT OUTPUT:\n[[%s]]\n" % (results_expect[len(results)]) + ) + print( + "Let the test write the file by rerunning with NM_TEST_REGENERATE=1" + ) + print( + "See howto in %s for details.\n" + % (PathConfiguration.canonical_script_filename()) + ) + sys.stdout.flush() + self.fail( + "Unexpected output of command, expected %s. Rerun test with NM_TEST_REGENERATE=1 to regenerate files" + % (filename) + ) + + if regenerate: + content_new = b"".join([r["content"] for r in results]) + if content_new != content_expect: + try: + with open(filename, "wb") as content_file: + content_file.write(content_new) + except Exception as e: + self.fail("Failure to write '%s': %s" % (filename, e)) + + if skip_test_for_l10n_diff: + # nmcli loads translations from the installation path. This failure commonly + # happens because you did not install the binary in the --prefix, before + # running the test. Hence, translations are not available or differ. + raise unittest.SkipTest( + "Skipped asserting for localized tests %s. Set NM_TEST_CLIENT_CHECK_L10N=1 to force fail." + % (",".join(skip_test_for_l10n_diff)) + ) + def nm_test(func): def f(self): self.ctx.srv_start() func(self) - self.ctx.run_post() + self.run_post() return f def nm_test_no_dbus(func): def f(self): func(self) - self.ctx.run_post() + self.run_post() return f @@ -2369,7 +2369,8 @@ class TestNmCloudSetup(unittest.TestCase): func(self) except Exception as e: error = e - self.ctx.run_post() + self.ctx.async_wait() + self.ctx.srv_shutdown() self.md_conn.close() p.stdin.close() From ecafa051dfb480f9bfa2fd2977150a05f3554185 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Mon, 13 Jan 2025 14:15:55 +0100 Subject: [PATCH 20/28] client/test: add ability to log pexpect traffic --- src/tests/client/test-client.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/tests/client/test-client.py b/src/tests/client/test-client.py index 8b4322e810..1d102312f0 100755 --- a/src/tests/client/test-client.py +++ b/src/tests/client/test-client.py @@ -86,6 +86,9 @@ ENV_NM_TEST_REGENERATE = "NM_TEST_REGENERATE" # numbers enabled. ENV_NM_TEST_WITH_LINENO = "NM_TEST_WITH_LINENO" +# Log pexpect output to stderr, for debuging +ENV_NM_TEST_LOG_PEXPECT = "NM_TEST_LOG_PEXPECT" + ENV_NM_TEST_ASAN_OPTIONS = "NM_TEST_ASAN_OPTIONS" ENV_NM_TEST_LSAN_OPTIONS = "NM_TEST_LSAN_OPTIONS" ENV_NM_TEST_UBSAN_OPTIONS = "NM_TEST_UBSAN_OPTIONS" @@ -689,7 +692,9 @@ class Util: argv, valgrind_log = Util.cmd_create_argv(cmd_path, args) env = Util.cmd_create_env(extra_env=extra_env) - pexp = pexpect.spawn(argv[0], argv[1:], timeout=10, env=env) + pexp = pexpect.spawn(argv[0], argv[1:], timeout=10, env=env, encoding="utf-8") + if conf.get(ENV_NM_TEST_LOG_PEXPECT): + pexp.logfile = sys.stderr pexp.str_last_chars = 100000 @@ -775,6 +780,8 @@ class Configuration: v = Util.is_bool(os.environ.get(ENV_NM_TEST_REGENERATE, None)) elif name == ENV_NM_TEST_WITH_LINENO: v = Util.is_bool(os.environ.get(ENV_NM_TEST_WITH_LINENO, None)) + elif name == ENV_NM_TEST_LOG_PEXPECT: + v = Util.is_bool(os.environ.get(ENV_NM_TEST_LOG_PEXPECT, None)) elif name == ENV_NM_TEST_VALGRIND: if self.get(ENV_NM_TEST_REGENERATE): v = False From 87b23669fad92ef3b80cf3aa8f31dfb0628e9aa1 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Mon, 18 Nov 2024 23:32:28 +0100 Subject: [PATCH 21/28] client/test: add nm-c-s OCI test Very basic. --- src/tests/client/test-client.py | 81 +++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/src/tests/client/test-client.py b/src/tests/client/test-client.py index 1d102312f0..25ed478b7b 100755 --- a/src/tests/client/test-client.py +++ b/src/tests/client/test-client.py @@ -2720,6 +2720,87 @@ class TestNmCloudSetup(unittest.TestCase): Util.valgrind_check_log(nmc.valgrind_log, "test_gcp") + @cloud_setup_test + def test_oci(self): + self._mock_devices() + + oci_meta = "/opc/v2/" + self._mock_path(oci_meta + "instance", "{}") + self._mock_path( + oci_meta + "vnics", + """ + [ + { + "macAddr": "%s", + "privateIp": "%s", + "subnetCidrBlock": "172.31.16.0/20", + "virtualRouterIp": "172.31.16.1", + "vlanTag": 810, + "vnicId": "ocid1.vnic.oc1.cz-adamov1.foobarbaz" + }, + { + "macAddr": "%s", + "privateIp": "%s", + "subnetCidrBlock": "172.31.166.0/20", + "virtualRouterIp": "172.31.166.1", + "vlanTag": 700, + "vnicId": "ocid1.vnic.oc1.uk-hogwarts.expelliarmus" + } + ] + """ + % ( + TestNmCloudSetup._mac1, + TestNmCloudSetup._ip1, + TestNmCloudSetup._mac2, + TestNmCloudSetup._ip2, + ), + ) + + # Run nm-cloud-setup for the first time + nmc = Util.cmd_call_pexpect( + ENV_NM_TEST_CLIENT_CLOUD_SETUP_PATH, + [], + { + "NM_CLOUD_SETUP_OCI_HOST": self.md_url, + "NM_CLOUD_SETUP_LOG": "trace", + "NM_CLOUD_SETUP_OCI": "yes", + }, + ) + + nmc.pexp.expect("provider oci detected") + nmc.pexp.expect("found interfaces: CC:00:00:00:00:01, CC:00:00:00:00:02") + nmc.pexp.expect("get-config: starting") + nmc.pexp.expect("get-config: success") + nmc.pexp.expect("meta data received") + # One of the devices has no IPv4 configuration to be modified + nmc.pexp.expect("device has no suitable applied connection. Skip") + # The other one was lacking an address set it up. + nmc.pexp.expect("some changes were applied for provider oci") + nmc.pexp.expect(pexpect.EOF) + + # Run nm-cloud-setup for the second time + nmc = Util.cmd_call_pexpect( + ENV_NM_TEST_CLIENT_CLOUD_SETUP_PATH, + [], + { + "NM_CLOUD_SETUP_OCI_HOST": self.md_url, + "NM_CLOUD_SETUP_LOG": "trace", + "NM_CLOUD_SETUP_OCI": "yes", + }, + ) + + nmc.pexp.expect("provider oci detected") + nmc.pexp.expect("found interfaces: CC:00:00:00:00:01, CC:00:00:00:00:02") + nmc.pexp.expect("get-config: starting") + nmc.pexp.expect("get-config: success") + nmc.pexp.expect("meta data received") + # No changes this time + nmc.pexp.expect('device needs no update to applied connection "con-eth0"') + nmc.pexp.expect("no changes were applied for provider oci") + nmc.pexp.expect(pexpect.EOF) + + Util.valgrind_check_log(nmc.valgrind_log, "test_oci") + ############################################################################### From c3861bc50aa6706bc5127ae597f548157f018cc5 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 29 Nov 2024 18:02:57 +0100 Subject: [PATCH 22/28] cloud-setup: add a sync wrapper around AddAndActivate These will be used to create the software devices. --- src/nm-cloud-setup/nm-cloud-setup-utils.c | 43 +++++++++++++++++++++++ src/nm-cloud-setup/nm-cloud-setup-utils.h | 5 +++ 2 files changed, 48 insertions(+) diff --git a/src/nm-cloud-setup/nm-cloud-setup-utils.c b/src/nm-cloud-setup/nm-cloud-setup-utils.c index 558f413f6a..314e401108 100644 --- a/src/nm-cloud-setup/nm-cloud-setup-utils.c +++ b/src/nm-cloud-setup/nm-cloud-setup-utils.c @@ -615,3 +615,46 @@ again: NM_SET_OUT(out_version_id_changed, FALSE); return TRUE; } + +/*****************************************************************************/ + +typedef struct { + GMainLoop *main_loop; + GError **error; + NMActiveConnection *active_connection; +} AddAndActivateData; + +static void +_nmcs_add_and_activate_cb(GObject *source, GAsyncResult *result, gpointer user_data) +{ + AddAndActivateData *data = user_data; + + data->active_connection = + nm_client_add_and_activate_connection_finish(NM_CLIENT(source), result, data->error); + g_main_loop_quit(data->main_loop); +} + +NMActiveConnection * +nmcs_add_and_activate(NMClient *client, + GCancellable *sigterm_cancellable, + NMConnection *connection, + GError **error) +{ + nm_auto_unref_gmainloop GMainLoop *main_loop = g_main_loop_new(NULL, FALSE); + AddAndActivateData data = { + .main_loop = main_loop, + .error = error, + }; + + nm_client_add_and_activate_connection_async(client, + connection, + NULL, + NULL, + sigterm_cancellable, + _nmcs_add_and_activate_cb, + &data); + + g_main_loop_run(main_loop); + + return data.active_connection; +} diff --git a/src/nm-cloud-setup/nm-cloud-setup-utils.h b/src/nm-cloud-setup/nm-cloud-setup-utils.h index c4662842d2..962053690e 100644 --- a/src/nm-cloud-setup/nm-cloud-setup-utils.h +++ b/src/nm-cloud-setup/nm-cloud-setup-utils.h @@ -153,6 +153,11 @@ NMConnection *nmcs_device_get_applied_connection(NMDevice *device, guint64 *version_id, GError **error); +NMActiveConnection *nmcs_add_and_activate(NMClient *client, + GCancellable *sigterm_cancellable, + NMConnection *connection, + GError **error); + gboolean nmcs_device_reapply(NMDevice *device, GCancellable *sigterm_cancellable, NMConnection *connection, From 04e426a7bf3d598143ee7275ffde91be22e77be0 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 3 Dec 2024 10:29:48 +0100 Subject: [PATCH 23/28] cloud-setup: s/_nmc_get_hwaddrs()/_nmc_get_ethernet_hwaddrs()/ Make it clear that this only works for Ethernet devices. --- src/nm-cloud-setup/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nm-cloud-setup/main.c b/src/nm-cloud-setup/main.c index 1de890be58..de03535ebd 100644 --- a/src/nm-cloud-setup/main.c +++ b/src/nm-cloud-setup/main.c @@ -242,7 +242,7 @@ _device_get_hwaddr(NMDeviceEthernet *device) } static char ** -_nmc_get_hwaddrs(NMClient *nmc) +_nmc_get_ethernet_hwaddrs(NMClient *nmc) { gs_unref_ptrarray GPtrArray *hwaddrs = NULL; const GPtrArray *devices; @@ -352,7 +352,7 @@ _get_config(GCancellable *sigterm_cancellable, NMCSProvider *provider, NMClient }; gs_strfreev char **hwaddrs = NULL; - hwaddrs = _nmc_get_hwaddrs(nmc); + hwaddrs = _nmc_get_ethernet_hwaddrs(nmc); nmcs_provider_get_config(provider, TRUE, From 04f0491a588af1a2f20e900cdeb8cf7b7a635388 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 3 Dec 2024 10:28:16 +0100 Subject: [PATCH 24/28] cloud-setup: make _device_get_hwaddr() work with all kinds of devices We'll have Vlans and MacVlans soon, and those don't have permanent addresses. --- src/nm-cloud-setup/main.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/nm-cloud-setup/main.c b/src/nm-cloud-setup/main.c index de03535ebd..e636aaf6bc 100644 --- a/src/nm-cloud-setup/main.c +++ b/src/nm-cloud-setup/main.c @@ -198,13 +198,14 @@ _map_interfaces_parse(void) } static const char * -_device_get_hwaddr(NMDeviceEthernet *device) +_device_get_hwaddr(NMDevice *device) { static const NMUtilsNamedValue *gl_map_interfaces_map = NULL; static gsize gl_initialized = 0; const NMUtilsNamedValue *map = NULL; - nm_assert(NM_IS_DEVICE_ETHERNET(device)); + nm_assert(NM_IS_DEVICE_ETHERNET(device) || NM_IS_DEVICE_MACVLAN(device) + || NM_IS_DEVICE_VLAN(device)); /* Network interfaces in cloud environments are identified by their permanent * MAC address. @@ -238,7 +239,11 @@ _device_get_hwaddr(NMDeviceEthernet *device) return NULL; } - return nm_device_ethernet_get_permanent_hw_address(device); + if (NM_IS_DEVICE_ETHERNET(device)) { + return nm_device_ethernet_get_permanent_hw_address(NM_DEVICE_ETHERNET(device)); + } else { + return nm_device_get_hw_address(device); + } } static char ** @@ -263,7 +268,7 @@ _nmc_get_ethernet_hwaddrs(NMClient *nmc) if (nm_device_get_state(device) < NM_DEVICE_STATE_UNAVAILABLE) continue; - hwaddr = _device_get_hwaddr(NM_DEVICE_ETHERNET(device)); + hwaddr = _device_get_hwaddr(device); if (!hwaddr) continue; @@ -305,7 +310,7 @@ _nmc_get_device_by_hwaddr(NMClient *nmc, const char *hwaddr) if (!NM_IS_DEVICE_ETHERNET(device)) continue; - hwaddr_dev = _device_get_hwaddr(NM_DEVICE_ETHERNET(device)); + hwaddr_dev = _device_get_hwaddr(device); if (!hwaddr_dev) continue; From daef3b7b3f0cfaa1df2f54b1525f51f918eaf162 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 19 Nov 2024 12:30:05 +0100 Subject: [PATCH 25/28] cloud-setup: lookup device by MAC + type instead of just MAC This will be useful for updating configuration of Vlans and MacVlans, some of having same MAC addresses as devices of other type. --- src/nm-cloud-setup/main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/nm-cloud-setup/main.c b/src/nm-cloud-setup/main.c index e636aaf6bc..bd44fbf16d 100644 --- a/src/nm-cloud-setup/main.c +++ b/src/nm-cloud-setup/main.c @@ -295,7 +295,7 @@ _nmc_get_ethernet_hwaddrs(NMClient *nmc) } static NMDevice * -_nmc_get_device_by_hwaddr(NMClient *nmc, const char *hwaddr) +_nmc_get_device_by_hwaddr(NMClient *nmc, NMDeviceType device_type, const char *hwaddr) { const GPtrArray *devices; guint i; @@ -307,7 +307,7 @@ _nmc_get_device_by_hwaddr(NMClient *nmc, const char *hwaddr) const char *hwaddr_dev; gs_free char *s = NULL; - if (!NM_IS_DEVICE_ETHERNET(device)) + if (nm_device_get_device_type(device) != device_type) continue; hwaddr_dev = _device_get_hwaddr(device); @@ -590,7 +590,7 @@ _config_one(SigTermData *sigterm_data, if (g_cancellable_is_cancelled(sigterm_data->cancellable)) return FALSE; - device = nm_g_object_ref(_nmc_get_device_by_hwaddr(nmc, hwaddr)); + device = nm_g_object_ref(_nmc_get_device_by_hwaddr(nmc, NM_DEVICE_TYPE_ETHERNET, hwaddr)); if (!device) { _LOGD("config device %s: skip because device not found", hwaddr); return FALSE; From 55ed4f7f6df9c86f0b7016c13cc2fbb1b35f13b6 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 19 Nov 2024 12:33:02 +0100 Subject: [PATCH 26/28] cloud-setup: skip connections unless given type mismatches Wired and Ipv4 always there, rest varies by connection type (Wired, Vlan, MacVlan). --- src/nm-cloud-setup/main.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/nm-cloud-setup/main.c b/src/nm-cloud-setup/main.c index bd44fbf16d..ec6d4f9a77 100644 --- a/src/nm-cloud-setup/main.c +++ b/src/nm-cloud-setup/main.c @@ -394,11 +394,10 @@ _nmc_skip_connection_by_user_data(NMConnection *connection) } static gboolean -_nmc_skip_connection_by_type(NMConnection *connection) +_nmc_skip_connection_by_type(NMConnection *connection, const char *connection_type) { - if (!nm_streq0(nm_connection_get_connection_type(connection), NM_SETTING_WIRED_SETTING_NAME)) + if (!nm_streq0(nm_connection_get_connection_type(connection), connection_type)) return TRUE; - if (!nm_connection_get_setting_ip4_config(connection)) return TRUE; @@ -639,7 +638,7 @@ try_again: return any_changes; } - if (_nmc_skip_connection_by_type(applied_connection)) { + if (_nmc_skip_connection_by_type(applied_connection, NM_SETTING_WIRED_SETTING_NAME)) { _LOGD("config device %s: device has no suitable applied connection. Skip", hwaddr); return any_changes; } From 6ff4b9e57cbc258e3f7cc810f4738c2dc8ad8076 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 29 Nov 2024 18:03:39 +0100 Subject: [PATCH 27/28] cloud-setup: create VLANs for multiple VNICs on OCI The idea is to create a pair of VLAN and MACVLAN with AddAndActivate if they are not present, and otherwise follow the ordinary (GetApplied & Reapply) procedure if the devices are already present. --- src/nm-cloud-setup/main.c | 300 ++++++++++++++++++++++++++------ src/tests/client/test-client.py | 1 - 2 files changed, 250 insertions(+), 51 deletions(-) diff --git a/src/nm-cloud-setup/main.c b/src/nm-cloud-setup/main.c index ec6d4f9a77..84396ec2a8 100644 --- a/src/nm-cloud-setup/main.c +++ b/src/nm-cloud-setup/main.c @@ -427,13 +427,23 @@ _nmc_mangle_connection(NMDevice *device, NM_SET_OUT(out_skipped_single_addr, FALSE); NM_SET_OUT(out_changed, FALSE); + if (strcmp(nm_connection_get_connection_type(connection), NM_SETTING_MACVLAN_SETTING_NAME) + == 0) { + /* The MACVLAN just sits in between, no L3 configuration on it */ + return; + } else if (strcmp(nm_connection_get_connection_type(connection), NM_SETTING_VLAN_SETTING_NAME) + != 0) { + /* Preserve existing L3 configuration if not a VLAN */ + if (device) { + if ((ac = nm_device_get_active_connection(device)) + && (remote_connection = NM_CONNECTION(nm_active_connection_get_connection(ac)))) + remote_s_ip = nm_connection_get_setting_ip4_config(remote_connection); + } + } + s_ip = nm_connection_get_setting_ip4_config(connection); nm_assert(NM_IS_SETTING_IP4_CONFIG(s_ip)); - if ((ac = nm_device_get_active_connection(device)) - && (remote_connection = NM_CONNECTION(nm_active_connection_get_connection(ac)))) - remote_s_ip = nm_connection_get_setting_ip4_config(remote_connection); - addrs_new = g_ptr_array_new_full(config_data->ipv4s_len, (GDestroyNotify) nm_ip_address_unref); rules_new = g_ptr_array_new_full(config_data->ipv4s_len, (GDestroyNotify) nm_ip_routing_rule_unref); @@ -566,54 +576,31 @@ _nmc_mangle_connection(NMDevice *device, /*****************************************************************************/ static gboolean -_config_one(SigTermData *sigterm_data, - NMClient *nmc, - const NMCSProviderGetConfigResult *result, - guint idx) +_config_existing(SigTermData *sigterm_data, + const NMCSProviderGetConfigIfaceData *config_data, + NMClient *nmc, + const NMCSProviderGetConfigResult *result, + const char *connection_type, + NMDevice *device) { - const NMCSProviderGetConfigIfaceData *config_data = result->iface_datas_arr[idx]; - const char *hwaddr = config_data->hwaddr; - gs_unref_object NMDevice *device = NULL; - gs_unref_object NMConnection *applied_connection = NULL; - guint64 applied_version_id; - gs_free_error GError *error = NULL; - gboolean changed; - gboolean skipped_single_addr; - gboolean version_id_changed; - guint try_count; - gboolean any_changes = FALSE; - gboolean maybe_no_preserved_external_ip; - - g_main_context_iteration(NULL, FALSE); - - if (g_cancellable_is_cancelled(sigterm_data->cancellable)) - return FALSE; - - device = nm_g_object_ref(_nmc_get_device_by_hwaddr(nmc, NM_DEVICE_TYPE_ETHERNET, hwaddr)); - if (!device) { - _LOGD("config device %s: skip because device not found", hwaddr); - return FALSE; - } - - if (!nmcs_provider_get_config_iface_data_is_valid(config_data)) { - _LOGD("config device %s: skip because meta data not successfully fetched", hwaddr); - return FALSE; - } - - if (config_data->iface_idx >= 100) { - /* since we use the iface_idx to select a table number, the range is limited from - * 0 to 99. Note that the providers are required to provide increasing numbers, - * so this means we bail out after the first 100 devices. */ - _LOGD("config device %s: skip because number of supported interfaces reached", hwaddr); - return FALSE; - } + const char *hwaddr = config_data->hwaddr; + gs_unref_object NMConnection *applied_connection = NULL; + guint64 applied_version_id; + gs_free_error GError *error = NULL; + gboolean changed; + gboolean skipped_single_addr; + gboolean version_id_changed; + guint try_count; + gboolean any_changes; + gboolean maybe_no_preserved_external_ip; _LOGD("config device %s: configuring \"%s\" (%s)...", hwaddr, nm_device_get_iface(device) ?: "/unknown/", nm_object_get_path(NM_OBJECT(device))); - try_count = 0; + try_count = 0; + any_changes = FALSE; try_again: g_clear_object(&applied_connection); @@ -638,7 +625,7 @@ try_again: return any_changes; } - if (_nmc_skip_connection_by_type(applied_connection, NM_SETTING_WIRED_SETTING_NAME)) { + if (_nmc_skip_connection_by_type(applied_connection, connection_type)) { _LOGD("config device %s: device has no suitable applied connection. Skip", hwaddr); return any_changes; } @@ -705,7 +692,7 @@ try_again: nm_connection_get_uuid(applied_connection), error->message); } - return any_changes; + return TRUE; } _LOGD("config device %s: connection \"%s\" (%s) reapplied", @@ -713,17 +700,230 @@ try_again: nm_connection_get_id(applied_connection), nm_connection_get_uuid(applied_connection)); + return TRUE; +} + +static gboolean +_config_ethernet(SigTermData *sigterm_data, + const NMCSProviderGetConfigIfaceData *config_data, + NMClient *nmc, + const NMCSProviderGetConfigResult *result) +{ + gs_unref_object NMDevice *device = NULL; + + device = nm_g_object_ref( + _nmc_get_device_by_hwaddr(nmc, NM_DEVICE_TYPE_ETHERNET, config_data->hwaddr)); + if (!device) { + _LOGD("config device %s: skip because device not found", config_data->hwaddr); + return FALSE; + } + + return _config_existing(sigterm_data, + config_data, + nmc, + result, + NM_SETTING_WIRED_SETTING_NAME, + device); +} + +static gboolean +_oci_new_vlan_dev(SigTermData *sigterm_data, + const NMCSProviderGetConfigIfaceData *config_data, + NMClient *nmc, + const NMCSProviderGetConfigResult *result, + const char *connection_type, + const char *parent_hwaddr) +{ + const char *hwaddr = config_data->hwaddr; + gs_unref_object NMConnection *connection = NULL; + gs_unref_object NMActiveConnection *active_connection = NULL; + gs_free_error GError *error = NULL; + gs_free char *macvlan_name = NULL; + gs_free char *connection_id = NULL; + char *ifname = NULL; + const char *ip4_config_method; + NMSettingUser *s_user; + + connection = nm_simple_connection_new(); + + macvlan_name = g_strdup_printf("macvlan%ld", config_data->iface_idx); + connection_id = g_strdup_printf("%s%ld", connection_type, config_data->iface_idx); + + if (strcmp(connection_type, NM_SETTING_MACVLAN_SETTING_NAME) == 0) { + nm_connection_add_setting(connection, + g_object_new(NM_TYPE_SETTING_MACVLAN, + NM_SETTING_MACVLAN_MODE, + NM_SETTING_MACVLAN_MODE_VEPA, + NULL)); + nm_connection_add_setting(connection, + g_object_new(NM_TYPE_SETTING_IP6_CONFIG, + NM_SETTING_IP_CONFIG_METHOD, + NM_SETTING_IP6_CONFIG_METHOD_DISABLED, + NULL)); + ip4_config_method = NM_SETTING_IP4_CONFIG_METHOD_DISABLED; + ifname = macvlan_name; + } else if (strcmp(connection_type, NM_SETTING_VLAN_SETTING_NAME) == 0) { + nm_connection_add_setting(connection, + g_object_new(NM_TYPE_SETTING_VLAN, + NM_SETTING_VLAN_PARENT, + macvlan_name, + NM_SETTING_VLAN_ID, + config_data->priv.oci.vlan_tag, + NULL)); + ip4_config_method = NM_SETTING_IP4_CONFIG_METHOD_MANUAL; + } else { + g_return_val_if_reached(FALSE); + } + + nm_connection_add_setting(connection, + g_object_new(NM_TYPE_SETTING_CONNECTION, + NM_SETTING_CONNECTION_ID, + connection_id, + NM_SETTING_CONNECTION_TYPE, + connection_type, + NM_SETTING_CONNECTION_INTERFACE_NAME, + ifname, + NULL)); + nm_connection_add_setting(connection, + g_object_new(NM_TYPE_SETTING_IP4_CONFIG, + NM_SETTING_IP_CONFIG_METHOD, + ip4_config_method, + NULL)); + nm_connection_add_setting(connection, + g_object_new(NM_TYPE_SETTING_WIRED, + NM_SETTING_WIRED_MAC_ADDRESS, + parent_hwaddr, + NM_SETTING_WIRED_CLONED_MAC_ADDRESS, + hwaddr, + NULL)); + + s_user = nm_setting_user_new(); + nm_connection_add_setting(connection, s_user); + nm_setting_user_set_data(NM_SETTING_USER(s_user), NM_USER_TAG_ORIGIN, "nm-cloud-setup", NULL); + + _nmc_mangle_connection(NULL, connection, result, config_data, NULL, NULL); + + _LOGD("config device %s: creating %s connection for VLAN %d on %s...", + hwaddr, + ifname ?: connection_type, + config_data->priv.oci.vlan_tag, + parent_hwaddr); + + active_connection = nmcs_add_and_activate(nmc, NULL, connection, &error); + if (active_connection == NULL) { + if (!nm_utils_error_is_cancelled(error)) { + _LOGD("config device %s: failure to activate connection: %s", hwaddr, error->message); + } + return FALSE; + } + + _LOGD("config device %s: connection \"%s\" (%s) created", + hwaddr, + nm_active_connection_get_id(active_connection), + nm_active_connection_get_uuid(active_connection)); + + return TRUE; +} + +static gboolean +_oci_config_vnic_dev(SigTermData *sigterm_data, + const NMCSProviderGetConfigIfaceData *config_data, + NMClient *nmc, + const NMCSProviderGetConfigResult *result, + NMDeviceType device_type, + const char *connection_type, + const char *parent_hwaddr) +{ + gs_unref_object NMDevice *device = NULL; + + device = nm_g_object_ref(_nmc_get_device_by_hwaddr(nmc, device_type, config_data->hwaddr)); + if (device) { + /* There is a device. Modify and reapply the currently applied connection. */ + return _config_existing(sigterm_data, config_data, nmc, result, connection_type, device); + } else { + /* There is no device, but we're configuring a VLAN. + * We can just go ahead and create one with a new connection. */ + return _oci_new_vlan_dev(sigterm_data, + config_data, + nmc, + result, + connection_type, + parent_hwaddr); + } +} + +static gboolean +_config_one(SigTermData *sigterm_data, + NMCSProvider *provider, + NMClient *nmc, + const NMCSProviderGetConfigResult *result, + guint idx) +{ + const NMCSProviderGetConfigIfaceData *config_data = result->iface_datas_arr[idx]; + gboolean any_changes; + + g_main_context_iteration(NULL, FALSE); + + if (g_cancellable_is_cancelled(sigterm_data->cancellable)) + return FALSE; + + if (!nmcs_provider_get_config_iface_data_is_valid(config_data)) { + _LOGD("config device %s: skip because meta data not successfully fetched", + config_data->hwaddr); + return FALSE; + } + + if (config_data->iface_idx >= 100) { + /* since we use the iface_idx to select a table number, the range is limited from + * 0 to 99. Note that the providers are required to provide increasing numbers, + * so this means we bail out after the first 100 devices. */ + _LOGD("config device %s: skip because number of supported interfaces reached", + config_data->hwaddr); + return FALSE; + } + + if (NMCS_IS_PROVIDER_OCI(provider) && config_data->priv.oci.vlan_tag != 0) { + if (config_data->priv.oci.parent_hwaddr == NULL) { + _LOGW("config device %s: has vlan id %d but no parent device", + config_data->hwaddr, + config_data->priv.oci.vlan_tag); + return FALSE; + } + + /* MACVLAN first, because VLAN is on top of it. */ + any_changes = _oci_config_vnic_dev(sigterm_data, + config_data, + nmc, + result, + NM_DEVICE_TYPE_MACVLAN, + NM_SETTING_MACVLAN_SETTING_NAME, + config_data->priv.oci.parent_hwaddr); + any_changes += _oci_config_vnic_dev(sigterm_data, + config_data, + nmc, + result, + NM_DEVICE_TYPE_VLAN, + NM_SETTING_VLAN_SETTING_NAME, + config_data->hwaddr); + + } else { + any_changes = _config_ethernet(sigterm_data, config_data, nmc, result); + } + return any_changes; } static gboolean -_config_all(SigTermData *sigterm_data, NMClient *nmc, const NMCSProviderGetConfigResult *result) +_config_all(SigTermData *sigterm_data, + NMCSProvider *provider, + NMClient *nmc, + const NMCSProviderGetConfigResult *result) { gboolean any_changes = FALSE; guint i; for (i = 0; i < result->n_iface_datas; i++) { - if (_config_one(sigterm_data, nmc, result, i)) + if (_config_one(sigterm_data, provider, nmc, result, i)) any_changes = TRUE; } @@ -808,7 +1008,7 @@ main(int argc, const char *const *argv) if (!result) goto done; - if (_config_all(&sigterm_data, nmc, result)) + if (_config_all(&sigterm_data, provider, nmc, result)) _LOGI("some changes were applied for provider %s", nmcs_provider_get_name(provider)); else _LOGD("no changes were applied for provider %s", nmcs_provider_get_name(provider)); diff --git a/src/tests/client/test-client.py b/src/tests/client/test-client.py index 25ed478b7b..03b0d1b7d4 100755 --- a/src/tests/client/test-client.py +++ b/src/tests/client/test-client.py @@ -2801,7 +2801,6 @@ class TestNmCloudSetup(unittest.TestCase): Util.valgrind_check_log(nmc.valgrind_log, "test_oci") - ############################################################################### From 9b258faab45a072e80d0c8e775505885bb704c4d Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 3 Dec 2024 10:48:13 +0100 Subject: [PATCH 28/28] client/test: add test for VLANs on OCI --- src/tests/client/test-client.py | 109 ++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/src/tests/client/test-client.py b/src/tests/client/test-client.py index 03b0d1b7d4..989f743295 100755 --- a/src/tests/client/test-client.py +++ b/src/tests/client/test-client.py @@ -2801,6 +2801,115 @@ class TestNmCloudSetup(unittest.TestCase): Util.valgrind_check_log(nmc.valgrind_log, "test_oci") + @cloud_setup_test + def test_oci_vlans(self): + self._mock_devices() + + oci_meta = "/opc/v2/" + self._mock_path(oci_meta + "instance", "{}") + self._mock_path( + oci_meta + "vnics", + """ + [ + { + "macAddr": "%s", + "privateIp": "%s", + "subnetCidrBlock": "172.31.16.0/20", + "virtualRouterIp": "172.31.16.1", + "vlanTag": 0, + "nicIndex": 0, + "vnicId": "ocid1.vnic.oc1.cz-adamov1.foobarbaz" + }, + { + "macAddr": "%s", + "privateIp": "%s", + "subnetCidrBlock": "172.31.166.0/20", + "virtualRouterIp": "172.31.166.1", + "vlanTag": 0, + "nicIndex": 1, + "vnicId": "ocid1.vnic.oc1.uk-hogwarts.expelliarmus" + }, + { + "macAddr": "C0:00:00:00:00:10", + "privateIp": "172.31.10.10", + "subnetCidrBlock": "172.31.10.0/20", + "virtualRouterIp": "172.31.10.1", + "vlanTag": 700, + "nicIndex": 0, + "vnicId": "ocid1.vnic.oc1.uk-hogwarts.keka" + } + ] + """ + % ( + TestNmCloudSetup._mac1, + TestNmCloudSetup._ip1, + TestNmCloudSetup._mac2, + TestNmCloudSetup._ip2, + ), + ) + + # Run nm-cloud-setup for the first time + nmc = Util.cmd_call_pexpect( + ENV_NM_TEST_CLIENT_CLOUD_SETUP_PATH, + [], + { + "NM_CLOUD_SETUP_OCI_HOST": self.md_url, + "NM_CLOUD_SETUP_LOG": "trace", + "NM_CLOUD_SETUP_OCI": "yes", + }, + ) + + nmc.pexp.expect("provider oci detected") + nmc.pexp.expect("found interfaces: CC:00:00:00:00:01, CC:00:00:00:00:02") + nmc.pexp.expect("get-config: starting") + nmc.pexp.expect("get-config: success") + nmc.pexp.expect("meta data received") + + # No configuration for the ethernets + nmc.pexp.expect('configuring "eth0"') + nmc.pexp.expect("device has no suitable applied connection. Skip") + + # Setting up the VLAN + nmc.pexp.expect( + "creating macvlan2 connection for VLAN 700 on CC:00:00:00:00:01..." + ) + nmc.pexp.expect("creating vlan connection for VLAN 700 on C0:00:00:00:00:10...") + + nmc.pexp.expect("some changes were applied for provider oci") + nmc.pexp.expect(pexpect.EOF) + + # TODO: Actually check the contents of the connection + # Probably needs changes to the mock service API + conn_macvlan = self.ctx.srv.findConnections(con_id="connection-3") + assert conn_macvlan is not None + conn_vlan = self.ctx.srv.findConnections(con_id="connection-4") + assert conn_vlan is not None + + # Run nm-cloud-setup for the second time + nmc = Util.cmd_call_pexpect( + ENV_NM_TEST_CLIENT_CLOUD_SETUP_PATH, + [], + { + "NM_CLOUD_SETUP_OCI_HOST": self.md_url, + "NM_CLOUD_SETUP_LOG": "trace", + "NM_CLOUD_SETUP_OCI": "yes", + }, + ) + + # Just the same ol' thing, just no changes this time + nmc.pexp.expect("provider oci detected") + nmc.pexp.expect("found interfaces: CC:00:00:00:00:01, CC:00:00:00:00:02") + nmc.pexp.expect("get-config: starting") + nmc.pexp.expect("get-config: success") + nmc.pexp.expect("meta data received") + nmc.pexp.expect('configuring "eth0"') + nmc.pexp.expect("device has no suitable applied connection. Skip") + nmc.pexp.expect("no changes were applied for provider oci") + nmc.pexp.expect(pexpect.EOF) + + Util.valgrind_check_log(nmc.valgrind_log, "test_oci_vlans") + + ###############################################################################