From fd6f48ec357f8f2d88a33fcf2891f01be65a1d81 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 9 Jun 2023 10:13:12 +0200 Subject: [PATCH 1/6] device: generic: make type-description property read-only The property is not written anywhere, make it read-only. --- src/core/devices/nm-device-generic.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/src/core/devices/nm-device-generic.c b/src/core/devices/nm-device-generic.c index c62c7ba11b..f3ac16e8b7 100644 --- a/src/core/devices/nm-device-generic.c +++ b/src/core/devices/nm-device-generic.c @@ -128,22 +128,6 @@ get_property(GObject *object, guint prop_id, GValue *value, GParamSpec *pspec) } } -static void -set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *pspec) -{ - NMDeviceGeneric *self = NM_DEVICE_GENERIC(object); - NMDeviceGenericPrivate *priv = NM_DEVICE_GENERIC_GET_PRIVATE(self); - - switch (prop_id) { - case PROP_TYPE_DESCRIPTION: - priv->type_description = g_value_dup_string(value); - break; - default: - G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); - break; - } -} - /*****************************************************************************/ static void @@ -212,7 +196,6 @@ nm_device_generic_class_init(NMDeviceGenericClass *klass) object_class->constructor = constructor; object_class->dispose = dispose; object_class->get_property = get_property; - object_class->set_property = set_property; dbus_object_class->interface_infos = NM_DBUS_INTERFACE_INFOS(&interface_info_device_generic); @@ -231,7 +214,7 @@ nm_device_generic_class_init(NMDeviceGenericClass *klass) "", "", NULL, - G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS); + G_PARAM_READABLE | G_PARAM_STATIC_STRINGS); g_object_class_install_properties(object_class, _PROPERTY_ENUMS_LAST, obj_properties); } From 3ea19523ee9865d7210b8da1fe2b6b7fa5011825 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 9 Jun 2023 10:15:40 +0200 Subject: [PATCH 2/6] device: generic: make type-description const The type is initialized from nm_platform_link_get_type_name(), which returns a static string; there is no need to duplicate the string. --- src/core/devices/nm-device-generic.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/src/core/devices/nm-device-generic.c b/src/core/devices/nm-device-generic.c index f3ac16e8b7..c0dcf0de69 100644 --- a/src/core/devices/nm-device-generic.c +++ b/src/core/devices/nm-device-generic.c @@ -16,7 +16,7 @@ NM_GOBJECT_PROPERTIES_DEFINE_BASE(PROP_TYPE_DESCRIPTION, ); typedef struct { - char *type_description; + const char *type_description; } NMDeviceGenericPrivate; struct _NMDeviceGeneric { @@ -64,11 +64,11 @@ realize_start_notify(NMDevice *device, const NMPlatformLink *plink) NM_DEVICE_CLASS(nm_device_generic_parent_class)->realize_start_notify(device, plink); - nm_clear_g_free(&priv->type_description); ifindex = nm_device_get_ip_ifindex(NM_DEVICE(self)); - if (ifindex > 0) + if (ifindex > 0) { priv->type_description = - g_strdup(nm_platform_link_get_type_name(nm_device_get_platform(device), ifindex)); + nm_platform_link_get_type_name(nm_device_get_platform(device), ifindex); + } } static gboolean @@ -164,17 +164,6 @@ nm_device_generic_new(const NMPlatformLink *plink, gboolean nm_plugin_missing) NULL); } -static void -dispose(GObject *object) -{ - NMDeviceGeneric *self = NM_DEVICE_GENERIC(object); - NMDeviceGenericPrivate *priv = NM_DEVICE_GENERIC_GET_PRIVATE(self); - - nm_clear_g_free(&priv->type_description); - - G_OBJECT_CLASS(nm_device_generic_parent_class)->dispose(object); -} - static const NMDBusInterfaceInfoExtended interface_info_device_generic = { .parent = NM_DEFINE_GDBUS_INTERFACE_INFO_INIT( NM_DBUS_INTERFACE_DEVICE_GENERIC, @@ -194,7 +183,6 @@ nm_device_generic_class_init(NMDeviceGenericClass *klass) NMDeviceClass *device_class = NM_DEVICE_CLASS(klass); object_class->constructor = constructor; - object_class->dispose = dispose; object_class->get_property = get_property; dbus_object_class->interface_infos = NM_DBUS_INTERFACE_INFOS(&interface_info_device_generic); From adef815219fa96d3cae410819852e2895f2f7158 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 5 Jun 2023 14:30:44 +0200 Subject: [PATCH 3/6] device: add comment about return value in nm_device_get_type_description() --- src/core/devices/nm-device.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 1b6b555794..edd8ef5f13 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -5405,8 +5405,8 @@ nm_device_get_type_description(NMDevice *self) g_return_val_if_fail(self != NULL, NULL); /* Beware: this function should return the same - * value as nm_device_get_type_description() in libnm. */ - + * value as nm_device_get_type_description() in libnm. + * The returned string is static or interned */ return NM_DEVICE_GET_CLASS(self)->get_type_description(self); } From 749ebef0d9d05935bf373999c1e5bbdc5f291f7d Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 22 May 2023 17:53:47 +0200 Subject: [PATCH 4/6] device: add nm_device_get_type_desc_for_log() When logging, messages include the interface name to specify what device they refer to. In most case the interface name is unique. There are some devices that don't have a kernel link associated, and their interface name is not guaranteed to be unique. This is currently the case for OVS bridges and OVS ports. When reading a log with duplicate interface names, it is difficult to understand what is happening. And this is made worse by the fact that it is common practice to assign the same name to all devices in a OVS hierarchy (bridge, port, interface). To make logs unambiguous, we want to print the device type together with the name; however we don't want to *always* print the type because in most cases it's not useful and it would consume valuable real estate on the screen. Adopt a simple heuristic of showing the type only for OVS devices. This commit adds a helper function to return the device type to show in logs, when it is needed. --- src/core/devices/nm-device.c | 18 ++++++++++++++++++ src/core/devices/nm-device.h | 1 + 2 files changed, 19 insertions(+) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index edd8ef5f13..6c9f93808c 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -5399,6 +5399,24 @@ nm_device_get_type_desc(NMDevice *self) return NM_DEVICE_GET_PRIVATE(self)->type_desc; } +const char * +nm_device_get_type_desc_for_log(NMDevice *self) +{ + const char *type; + + type = nm_device_get_type_desc(self); + + /* Some OVS device types (ports and bridges) are not backed by a kernel link, and + * they can have the same name of another device of a different type. In fact, it's + * quite common to assign the same name to the OVS bridge, the OVS port and the OVS + * interface. For this reason, also log the type in case of OVS devices to make the + * log message unambiguous. */ + if (NM_STR_HAS_PREFIX(type, "Open vSwitch")) + return type; + + return NULL; +} + const char * nm_device_get_type_description(NMDevice *self) { diff --git a/src/core/devices/nm-device.h b/src/core/devices/nm-device.h index e22dbd07a9..35a8d1cd49 100644 --- a/src/core/devices/nm-device.h +++ b/src/core/devices/nm-device.h @@ -463,6 +463,7 @@ int nm_device_get_ip_ifindex(const NMDevice *dev); const char *nm_device_get_driver(NMDevice *dev); const char *nm_device_get_driver_version(NMDevice *dev); const char *nm_device_get_type_desc(NMDevice *dev); +const char *nm_device_get_type_desc_for_log(NMDevice *dev); const char *nm_device_get_type_description(NMDevice *dev); NMDeviceType nm_device_get_device_type(NMDevice *dev); NMLinkType nm_device_get_link_type(NMDevice *dev); From cb423ae7ac66a04db7f47f5f33d4546945a912f5 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 22 May 2023 17:55:55 +0200 Subject: [PATCH 5/6] dhcp: store the device type for logging Arguably, a kernel link is needed for DHCP and so the interface name univocally identifies a device (for example, the OVS interface). But for consistency and clarity, store the device type to be used for logging. --- src/core/devices/nm-device.c | 2 ++ src/core/dhcp/nm-dhcp-client.c | 10 ++++++++++ src/core/dhcp/nm-dhcp-client.h | 5 +++++ 3 files changed, 17 insertions(+) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 6c9f93808c..e5290e796d 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -10676,6 +10676,7 @@ _dev_ipdhcpx_start(NMDevice *self, int addr_family) .addr_family = AF_INET, .l3cfg = nm_device_get_l3cfg(self), .iface = nm_device_get_ip_iface(self), + .iface_type_log = nm_device_get_type_desc_for_log(self), .uuid = nm_connection_get_uuid(connection), .hwaddr = hwaddr, .bcast_hwaddr = bcast_hwaddr, @@ -10713,6 +10714,7 @@ _dev_ipdhcpx_start(NMDevice *self, int addr_family) .addr_family = AF_INET6, .l3cfg = nm_device_get_l3cfg(self), .iface = nm_device_get_ip_iface(self), + .iface_type_log = nm_device_get_type_desc_for_log(self), .uuid = nm_connection_get_uuid(connection), .send_hostname = nm_setting_ip_config_get_dhcp_send_hostname(s_ip), .hostname = nm_setting_ip_config_get_dhcp_hostname(s_ip), diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index b10ce41063..6978bd3cb0 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -196,6 +196,14 @@ nm_dhcp_client_get_iface(NMDhcpClient *self) return priv->config.iface; } +const char * +nm_dhcp_client_get_iface_type_for_log(NMDhcpClient *self) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + + return priv->config.iface_type_log; +} + NMDedupMultiIndex * nm_dhcp_client_get_multi_idx(NMDhcpClient *self) { @@ -1823,6 +1831,7 @@ config_init(NMDhcpClientConfig *config, const NMDhcpClientConfig *src) nm_g_bytes_ref(config->client_id); config->iface = g_strdup(config->iface); + config->iface_type_log = g_strdup(config->iface_type_log); config->uuid = g_strdup(config->uuid); config->anycast_address = g_strdup(config->anycast_address); config->hostname = g_strdup(config->hostname); @@ -1885,6 +1894,7 @@ config_clear(NMDhcpClientConfig *config) nm_clear_pointer(&config->client_id, g_bytes_unref); nm_clear_g_free((gpointer *) &config->iface); + nm_clear_g_free((gpointer *) &config->iface_type_log); nm_clear_g_free((gpointer *) &config->uuid); nm_clear_g_free((gpointer *) &config->anycast_address); nm_clear_g_free((gpointer *) &config->hostname); diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h index f21dc9133a..903ea6acc2 100644 --- a/src/core/dhcp/nm-dhcp-client.h +++ b/src/core/dhcp/nm-dhcp-client.h @@ -103,6 +103,10 @@ typedef struct { const char *iface; + /* Interface type for logging; only set for some devices whose names can be + * ambiguous. */ + const char *iface_type_log; + /* The hardware address */ GBytes *hwaddr; @@ -264,6 +268,7 @@ gboolean nm_dhcp_client_server_id_is_rejected(NMDhcpClient *self, gconstpointer int nm_dhcp_client_get_addr_family(NMDhcpClient *self); const char *nm_dhcp_client_get_iface(NMDhcpClient *self); +const char *nm_dhcp_client_get_iface_type_for_log(NMDhcpClient *self); NMDedupMultiIndex *nm_dhcp_client_get_multi_idx(NMDhcpClient *self); int nm_dhcp_client_get_ifindex(NMDhcpClient *self); From 680c95ddd20253485658dbee4a76ad66d2517c6a Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 22 May 2023 17:56:22 +0200 Subject: [PATCH 6/6] core: log the device type when it can be ambiguous Use the nm_device_get_type_desc_for_log() helper function defined earlier to show the device type when it can be ambiguous. With this, the log becomes a bit more explicative when there are OVS devices involved: device (ovs-br)[Open vSwitch Bridge]: state change: ip-config -> ip-check (reason 'none', sys-iface-state: 'managed') device (ovs-br)[Open vSwitch Port]: state change: ip-check -> secondaries (reason 'none', sys-iface-state: 'managed') device (ovs-br)[Open vSwitch Port]: state change: secondaries -> activated (reason 'none', sys-iface-state: 'managed') device (ovs-br)[Open vSwitch Port]: Activation: successful, device activated. device (ovs-br)[Open vSwitch Bridge]: state change: ip-check -> secondaries (reason 'none', sys-iface-state: 'managed') device (ovs-br)[Open vSwitch Bridge]: state change: secondaries -> activated (reason 'none', sys-iface-state: 'managed') device (ovs-br)[Open vSwitch Bridge]: Activation: successful, device activated. device (ovs-br)[Open vSwitch Interface]: state change: unmanaged -> unavailable (reason 'managed', sys-iface-state: 'external') device (ovs-br)[Open vSwitch Interface]: state change: unavailable -> disconnected (reason 'none', sys-iface-state: 'managed') device (ovs-br)[Open vSwitch Interface]: Activation: starting connection 'ovs-interface+' (d3d429b1-3193-4462-a17a-034255c43776) instead of: device (ovs-br): state change: ip-config -> ip-check (reason 'none', sys-iface-state: 'managed') device (ovs-br): state change: ip-check -> secondaries (reason 'none', sys-iface-state: 'managed') device (ovs-br): state change: secondaries -> activated (reason 'none', sys-iface-state: 'managed') device (ovs-br): Activation: successful, device activated. device (ovs-br): state change: ip-check -> secondaries (reason 'none', sys-iface-state: 'managed') device (ovs-br): state change: secondaries -> activated (reason 'none', sys-iface-state: 'managed') device (ovs-br): Activation: successful, device activated. device (ovs-br): state change: unmanaged -> unavailable (reason 'managed', sys-iface-state: 'external') device (ovs-br): state change: unavailable -> disconnected (reason 'none', sys-iface-state: 'managed') device (ovs-br): Activation: starting connection 'ovs-interface+' (d3d429b1-3193-4462-a17a-034255c43776) --- src/core/devices/nm-device-logging.h | 44 ++++++++++++++------------ src/core/dhcp/nm-dhcp-client-logging.h | 7 ++-- 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/src/core/devices/nm-device-logging.h b/src/core/devices/nm-device-logging.h index 0ea332d8f4..53330b5e92 100644 --- a/src/core/devices/nm-device-logging.h +++ b/src/core/devices/nm-device-logging.h @@ -19,27 +19,29 @@ #undef _NMLOG_ENABLED #define _NMLOG_ENABLED(level, domain) (nm_logging_enabled((level), (domain))) -#define _NMLOG(level, domain, ...) \ - G_STMT_START \ - { \ - const NMLogLevel _level = (level); \ - const NMLogDomain _domain = (domain); \ - \ - if (nm_logging_enabled(_level, _domain)) { \ - typeof(*self) *const _self = (self); \ - const char *const _ifname = _nm_device_get_iface(_NM_DEVICE_CAST(_self)); \ - \ - nm_log_obj(_level, \ - _domain, \ - _ifname, \ - NULL, \ - _self, \ - "device", \ - "%s%s%s: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ - NM_PRINT_FMT_QUOTED(_ifname, "(", _ifname, ")", "[null]") \ - _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ - } \ - } \ +#define _NMLOG(level, domain, ...) \ + G_STMT_START \ + { \ + const NMLogLevel _level = (level); \ + const NMLogDomain _domain = (domain); \ + \ + if (nm_logging_enabled(_level, _domain)) { \ + typeof(*self) *const _self = (self); \ + const char *const _ifname = _nm_device_get_iface(_NM_DEVICE_CAST(_self)); \ + const char *_type = nm_device_get_type_desc_for_log(_NM_DEVICE_CAST(_self)); \ + \ + nm_log_obj(_level, \ + _domain, \ + _ifname, \ + NULL, \ + _self, \ + "device", \ + "%s%s%s%s%s%s: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ + NM_PRINT_FMT_QUOTED(_ifname, "(", _ifname, ")", "[null]"), \ + NM_PRINT_FMT_QUOTED(_type, "[", _type, "]", "") \ + _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ + } \ + } \ G_STMT_END #endif /* __NETWORKMANAGER_DEVICE_LOGGING_H__ */ diff --git a/src/core/dhcp/nm-dhcp-client-logging.h b/src/core/dhcp/nm-dhcp-client-logging.h index 2b0d8d06c0..3a2b927add 100644 --- a/src/core/dhcp/nm-dhcp-client-logging.h +++ b/src/core/dhcp/nm-dhcp-client-logging.h @@ -41,16 +41,19 @@ _nm_dhcp_client_get_domain(NMDhcpClient *self) if (nm_logging_enabled(_level, _NMLOG_DOMAIN)) { \ NMDhcpClient *_self = (NMDhcpClient *) (self); \ const char *__ifname = _self ? nm_dhcp_client_get_iface(_self) : NULL; \ + const char *_type = nm_dhcp_client_get_iface_type_for_log(_self); \ const NMLogDomain _domain = _nm_dhcp_client_get_domain(_self); \ \ nm_log(_level, \ _domain, \ __ifname, \ NULL, \ - "%s%s%s%s%s: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ + "%s%s%s%s%s%s%s%s%s: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ _NMLOG_PREFIX_NAME, \ (_domain == LOGD_DHCP4 ? "4" : (_domain == LOGD_DHCP6 ? "6" : "")), \ - NM_PRINT_FMT_QUOTED(__ifname, " (", __ifname, ")", "") \ + (__ifname || _type) ? " " : "", \ + NM_PRINT_FMT_QUOTED(__ifname, "(", __ifname, ")", ""), \ + NM_PRINT_FMT_QUOTED(_type, "[", _type, "]", "") \ _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ } \ } \