From 8cb739031d0f129dca2dbefaf0bc38ca33216901 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 6 Dec 2022 14:57:50 +0100 Subject: [PATCH 1/2] device: use correct field "l3cfg_" to clear in dispose() The fields "l3cfg" and "l3cfg_" are union aliases. One of them is const, the other is not. The idea is that all places that modify the field need to use the special name "l3cfg_", and grepping for that will lead you to all the relevant places. This mistake happened, because g_clear_object() casts constness away. Fixes: 58287cbcc0c8 ('core: rework IP configuration in NetworkManager using layer 3 configuration') --- src/core/devices/nm-device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index e1569cdbab..5301436855 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -17891,7 +17891,7 @@ dispose(GObject *object) priv->sriov.next = NULL; } - g_clear_object(&priv->l3cfg); + g_clear_object(&priv->l3cfg_); g_clear_object(&priv->l3ipdata_4.ip_config); g_clear_object(&priv->l3ipdata_6.ip_config); From 78c8a5d0164f1203d448710d6a616ce7f3d4d0c9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 6 Dec 2022 14:35:15 +0100 Subject: [PATCH 2/2] glib-aux: reimplement g_clear_pointer() from glib 2.58+ in "nm-glib.h" compat header Glib 2.58+ improved the implementation of the g_clear_pointer() macro, and indirectly of g_clear_object(), which uses it. Note that we don't use the 2.58+ version, because our GLIB_VERSION_MAX_ALLOWED is too old. Also note that we don't use g_clear_pointer() directly. Instead, we have and use nm_clear_pointer() everywhere. Still, it would be nice if also g_clear_object() uses the improved variant. Arguably, this is less relevant, because g_clear_object() calls g_unref_object() which accepts a void pointer and thus there isn't much type-safety to gain. Still, there is a small gain, so do it. We could: 1) replace all uses of g_clear_object() with nm_clear_g_object() and outlaw both g_clear_object() and g_clear_pointer(). This is what's done for nm_clear_pointer(), which should be used instead of g_clear_pointer(). The advantage is that we don't monkey-patch glib (which might surprise users). The disadvantage is that g_clear_pointer() is well known, while nm_clear_pointer() is not. This is mitigated by the fact that nm_clear_pointer() behaves very similar to g_clear_pointer() and in all cases where you legally could use g_clear_pointer(), nm_clear_pointer() works to the same effect (but not vice versa). 2) silently redefine the glib helper to use our improved implementation. This is done for g_clear_error(), which is redefined to nm_clear_error(). The advantage is that it appears as if we would use glib functionality. The disadvantage is that this is not exactly the glib variant. This too is mitigated by the fact that our patched g_clear_error() should work the same, wherever you can legally use glib's variant (but not vice versa). Let's do 2). In this case, let g_clear_pointer() behaves exactly like glib 2.58+'s variant, and not like nm_clear_pointer(). This is to reduce any potential surprise. nm_clear_pointer() is still better. Still use that over g_clear_pointer(). This change is for g_clear_object(). --- src/libnm-glib-aux/nm-glib.h | 50 ++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/src/libnm-glib-aux/nm-glib.h b/src/libnm-glib-aux/nm-glib.h index 2dc86d7b4e..0436d13554 100644 --- a/src/libnm-glib-aux/nm-glib.h +++ b/src/libnm-glib-aux/nm-glib.h @@ -45,28 +45,40 @@ __g_type_ensure(GType type) /*****************************************************************************/ -#if !GLIB_CHECK_VERSION(2, 34, 0) +/* glib 2.58+ defines an improved, type-safe variant of g_clear_pointer(). Reimplement + * that. + * + * Note that we also have nm_clear_pointer() which is similar to g_clear_pointer() + * and also preferred throughout. However, we do use g_clear_object(), which is + * implemented via g_clear_pointer(). So while there are no immediate users of + * g_clear_pointer() (use nm_clear_pointer() instead), there are indirect users + * via g_clear_object(). + * + * Anyway. So the glib 2.58+ version of g_clear_pointer() is only used if GLIB_VERSION_MAX_ALLOWED + * is set, which we don't do. + * + * To get the type-safe variant, reimplement g_clear_pointer() below. This aims to + * be identical to the version of the macro in glib 2.58. + * + * Still, don't use g_clear_pointer() directly (use nm_clear_pointer()). This compat + * implementation exists only for g_clear_object(). + */ +#undef g_clear_pointer -#define g_clear_pointer(pp, destroy) \ - G_STMT_START \ - { \ - G_STATIC_ASSERT(sizeof *(pp) == sizeof(gpointer)); \ - /* Only one access, please */ \ - gpointer *_pp = (gpointer *) (pp); \ - gpointer _p; \ - /* This assignment is needed to avoid a gcc warning */ \ - GDestroyNotify _destroy = (GDestroyNotify) (destroy); \ - \ - _p = *_pp; \ - if (_p) { \ - *_pp = NULL; \ - _destroy(_p); \ - } \ - } \ +#define g_clear_pointer(pp, destroy) \ + G_STMT_START \ + { \ + typeof((pp)) _pp = (pp); \ + typeof(*_pp) _ptr = *_pp; \ + \ + G_STATIC_ASSERT(sizeof *(pp) == sizeof(gpointer)); \ + \ + *_pp = NULL; \ + if (_ptr) \ + (destroy)(_ptr); \ + } \ G_STMT_END -#endif - /*****************************************************************************/ #if !GLIB_CHECK_VERSION(2, 34, 0)