From 1b2b988ea95c5d7cc7d4e8bcee3ab2a325c62c52 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Jun 2015 14:41:35 +0200 Subject: [PATCH] platform: no longer expose udi field in NMPlatformLink The @udi field is not a static string, so any user of a NMPlatformLink instance must make sure not to use the field beyond the lifetime of the NMPlatformLink instance. As we pass on the platform link instance during platform changed events, this is hard to ensure for the subscriber of the signal -- because a call back into platform could invalidate/modify the object. Just not expose this field as part of the link instance. The few callers who actually needed it should instead call nm_platform_get_uid(). With that, the lifetime of the returned 'const char *' pointer is clearly defined. --- src/devices/nm-device-ethernet.c | 2 +- src/devices/nm-device.c | 8 +++++--- src/platform/nm-fake-platform.c | 14 +++++++++++++- src/platform/nm-linux-platform.c | 13 +++++++++++++ src/platform/nm-platform.c | 23 ++++++++++++++--------- src/platform/nm-platform.h | 8 ++------ src/platform/nmp-object.c | 15 +++------------ src/platform/tests/dump.c | 2 +- 8 files changed, 52 insertions(+), 33 deletions(-) diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c index a0aa02d1ac..70e9e70c47 100644 --- a/src/devices/nm-device-ethernet.c +++ b/src/devices/nm-device-ethernet.c @@ -1559,7 +1559,7 @@ link_changed (NMDevice *device, NMPlatformLink *info) NMDeviceEthernetPrivate *priv = NM_DEVICE_ETHERNET_GET_PRIVATE (self); NM_DEVICE_CLASS (nm_device_ethernet_parent_class)->link_changed (device, info); - if (!priv->subchan1 && info->udi) + if (!priv->subchan1 && info->initialized) _update_s390_subchannels (self); } diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 2a19619d8f..04472847be 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -1341,11 +1341,13 @@ device_link_changed (NMDevice *self, NMPlatformLink *info) NMUtilsIPv6IfaceId token_iid; gboolean ip_ifname_changed = FALSE; gboolean platform_unmanaged = FALSE; + const char *udi; - if (info->udi && g_strcmp0 (info->udi, priv->udi)) { + udi = nm_platform_link_get_udi (NM_PLATFORM_GET, info->ifindex); + if (udi && g_strcmp0 (udi, priv->udi)) { /* Update UDI to what udev gives us */ g_free (priv->udi); - priv->udi = g_strdup (info->udi); + priv->udi = g_strdup (udi); g_object_notify (G_OBJECT (self), NM_DEVICE_UDI); } @@ -8938,7 +8940,7 @@ set_property (GObject *object, guint prop_id, platform_device = g_value_get_pointer (value); if (platform_device) { g_free (priv->udi); - priv->udi = g_strdup (platform_device->udi); + priv->udi = g_strdup (nm_platform_link_get_udi (NM_PLATFORM_GET, platform_device->ifindex)); g_free (priv->iface); priv->iface = g_strdup (platform_device->name); priv->ifindex = platform_device->ifindex; diff --git a/src/platform/nm-fake-platform.c b/src/platform/nm-fake-platform.c index 042d98a282..36c6898b48 100644 --- a/src/platform/nm-fake-platform.c +++ b/src/platform/nm-fake-platform.c @@ -129,7 +129,7 @@ link_init (NMFakePlatformLink *device, int ifindex, int type, const char *name) device->link.type = type; device->link.kind = type_to_type_name (type); device->link.driver = type_to_type_name (type); - device->link.udi = device->udi = g_strdup_printf ("fake:%d", ifindex); + device->udi = g_strdup_printf ("fake:%d", ifindex); device->link.initialized = TRUE; device->ip6_lladdr = *nmtst_inet6_from_string (ip6_lladdr); if (name) @@ -543,6 +543,16 @@ link_get_dev_id (NMPlatform *platform, int ifindex) return 0; } +static const char * +link_get_udi (NMPlatform *platform, int ifindex) +{ + NMFakePlatformLink *device = link_get (platform, ifindex); + + if (!device) + return NULL; + return device->udi; +} + static gboolean link_get_wake_on_lan (NMPlatform *platform, int ifindex) { @@ -1511,6 +1521,8 @@ nm_fake_platform_class_init (NMFakePlatformClass *klass) platform_class->link_get_type_name = link_get_type_name; platform_class->link_get_unmanaged = link_get_unmanaged; + platform_class->link_get_udi = link_get_udi; + platform_class->link_set_up = link_set_up; platform_class->link_set_down = link_set_down; platform_class->link_set_arp = link_set_arp; diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index edb1addaa5..b42b40feda 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -2950,6 +2950,18 @@ link_get_ipv6_token (NMPlatform *platform, int ifindex, NMUtilsIPv6IfaceId *iid) return TRUE; } +static const char * +link_get_udi (NMPlatform *platform, int ifindex) +{ + const NMPObject *obj = cache_lookup_link (platform, ifindex); + + if ( !obj + || !obj->_link.netlink.is_in_netlink + || !obj->_link.udev.device) + return NULL; + return g_udev_device_get_sysfs_path (obj->_link.udev.device); +} + static gboolean link_get_user_ipv6ll_enabled (NMPlatform *platform, int ifindex) { @@ -4979,6 +4991,7 @@ nm_linux_platform_class_init (NMLinuxPlatformClass *klass) platform_class->link_is_connected = link_is_connected; platform_class->link_uses_arp = link_uses_arp; + platform_class->link_get_udi = link_get_udi; platform_class->link_get_ipv6_token = link_get_ipv6_token; platform_class->link_get_user_ipv6ll_enabled = link_get_user_ipv6ll_enabled; diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index f77ee93498..b5a2099012 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -915,6 +915,18 @@ nm_platform_link_get_ipv6_token (NMPlatform *self, int ifindex, NMUtilsIPv6Iface return FALSE; } +const char * +nm_platform_link_get_udi (NMPlatform *self, int ifindex) +{ + _CHECK_SELF (self, klass, FALSE); + reset_error (self); + + g_return_val_if_fail (ifindex >= 0, NULL); + + if (klass->link_get_udi) + return klass->link_get_udi (self, ifindex); + return NULL; +} /** * nm_platform_link_get_user_ip6vll_enabled: @@ -2533,7 +2545,7 @@ nm_platform_link_to_string (const NMPlatformLink *link) "%s%s" /* addr */ "%s%s" /* inet6_token */ "%s%s" /* driver */ - "%s%s", /* udi */ + , link->ifindex, link->name, parent, @@ -2552,9 +2564,7 @@ nm_platform_link_to_string (const NMPlatformLink *link) str_inet6_token ? " inet6token " : "", str_inet6_token ? str_inet6_token : "", link->driver ? " driver " : "", - link->driver ? link->driver : "", - link->udi ? " udi " : "", - link->udi ? link->udi : ""); + link->driver ? link->driver : ""); g_string_free (str_flags, TRUE); return _nm_platform_to_string_buffer; } @@ -2854,11 +2864,6 @@ nm_platform_link_cmp (const NMPlatformLink *a, const NMPlatformLink *b) _CMP_FIELD (a, b, inet6_addr_gen_mode_inv); _CMP_FIELD (a, b, inet6_token.is_valid); _CMP_FIELD_STR_INTERNED (a, b, kind); - - /* udi is not an interned string, but NMRefString. Hence, - * do a pointer comparison first. */ - _CMP_FIELD_STR_INTERNED (a, b, udi); - _CMP_FIELD_STR_INTERNED (a, b, driver); if (a->addr.len) _CMP_FIELD_MEMCMP_LEN (a, b, addr.data, a->addr.len); diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index a0addbfdd5..fb9ed34fec 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -95,12 +95,6 @@ struct _NMPlatformLink { /* NMPlatform initializes this field with a static string. */ const char *kind; - /* Beware: NMPlatform initializes this string with an allocated string - * (NMRefString). Handle it properly (i.e. don't keep a reference to it - * without incrementing the ref-counter). - * This property depends on @initialized. */ - const char *udi; - /* NMPlatform initializes this field with a static string. */ const char *driver; @@ -438,6 +432,7 @@ typedef struct { gboolean (*link_is_connected) (NMPlatform *, int ifindex); gboolean (*link_uses_arp) (NMPlatform *, int ifindex); + const char *(*link_get_udi) (NMPlatform *self, int ifindex); gboolean (*link_get_ipv6_token) (NMPlatform *, int ifindex, NMUtilsIPv6IfaceId *iid); gboolean (*link_get_user_ipv6ll_enabled) (NMPlatform *, int ifindex); @@ -627,6 +622,7 @@ gboolean nm_platform_link_is_connected (NMPlatform *self, int ifindex); gboolean nm_platform_link_uses_arp (NMPlatform *self, int ifindex); gboolean nm_platform_link_get_ipv6_token (NMPlatform *self, int ifindex, NMUtilsIPv6IfaceId *iid); +const char *nm_platform_link_get_udi (NMPlatform *self, int ifindex); gboolean nm_platform_link_get_user_ipv6ll_enabled (NMPlatform *self, int ifindex); gboolean nm_platform_link_set_user_ipv6ll_enabled (NMPlatform *self, int ifindex, gboolean enabled); diff --git a/src/platform/nmp-object.c b/src/platform/nmp-object.c index 38eef91452..2765d113d1 100644 --- a/src/platform/nmp-object.c +++ b/src/platform/nmp-object.c @@ -132,7 +132,6 @@ void _nmp_object_fixup_link_udev_fields (NMPObject *obj, gboolean use_udev) { const char *driver = NULL; - const char *udi = NULL; gboolean initialized = FALSE; nm_assert (NMP_OBJECT_GET_TYPE (obj) == OBJECT_TYPE_LINK); @@ -145,10 +144,9 @@ _nmp_object_fixup_link_udev_fields (NMPObject *obj, gboolean use_udev) driver = _link_get_driver (obj->_link.udev.device, obj->link.kind, obj->link.name); - if (obj->_link.udev.device) { - udi = g_udev_device_get_sysfs_path (obj->_link.udev.device); + if (obj->_link.udev.device) initialized = TRUE; - } else if (!use_udev) { + else if (!use_udev) { /* If we don't use udev, we immediately mark the link as initialized. * * For that, we consult @use_udev argument, that is cached via @@ -165,7 +163,6 @@ _nmp_object_fixup_link_udev_fields (NMPObject *obj, gboolean use_udev) } obj->link.driver = driver; - obj->link.udi = nm_ref_string_replace (obj->link.udi, udi); obj->link.initialized = initialized; } @@ -225,7 +222,6 @@ nmp_object_unref (NMPObject *obj) static void _vt_cmd_obj_dispose_link (NMPObject *obj) { - nm_ref_string_unref (obj->link.udi); g_clear_object (&obj->_link.udev.device); } @@ -587,10 +583,6 @@ _vt_cmd_obj_copy_link (NMPObject *dst, const NMPObject *src) if (src->_link.udev.device) g_object_ref (src->_link.udev.device); } - if (dst->link.udi != src->link.udi) { - nm_ref_string_unref (dst->link.udi); - nm_ref_string_ref (src->link.udi); - } dst->_link = src->_link; } @@ -1512,8 +1504,7 @@ nmp_cache_update_netlink (NMPCache *cache, NMPObject *obj, NMPObject **out_obj, * a use-case */ nm_assert (NMP_OBJECT_GET_TYPE (obj) != OBJECT_TYPE_LINK || ( !obj->_link.udev.device - && !obj->link.driver - && !obj->link.udi)); + && !obj->link.driver)); old = g_hash_table_lookup (cache->idx_main, obj); diff --git a/src/platform/tests/dump.c b/src/platform/tests/dump.c index 5187d266f6..575ce6fb34 100644 --- a/src/platform/tests/dump.c +++ b/src/platform/tests/dump.c @@ -44,7 +44,7 @@ dump_interface (NMPlatformLink *link) printf ("\n"); if (link->driver) printf (" driver: %s\n", link->driver); - printf (" UDI: %s\n", link->udi); + printf (" UDI: %s\n", nm_platform_link_get_udi (NM_PLATFORM_GET, link->ifindex)); if (!nm_platform_vlan_get_info (NM_PLATFORM_GET, link->ifindex, &vlan_parent, &vlan_id)) g_assert_not_reached (); if (vlan_parent)