From 4ad4a1fbeb60afa4ba1b0d925698fd9563399c4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Mon, 11 Sep 2023 11:40:37 +0200 Subject: [PATCH] nm-meta-setting-desc: refactor and comment when get_gtype is disallowed The get_gtype field in property_typ_data is intended to specify an enum type for properties that are really defined as (u)int in the NMSetting class. Specifying get_gtype for properties that are already defined as enum in the NMSetting class is rejected as a runtime error. However, the error message doesn't explain the reason. Put a code comment explaining the reason. Explaining it in a comment is actually enough because: - The error is a runtime assertion that indicates a programming error - The assertion is checked any time that the property is read or written, so it should always be detected at developing time when doing changes to the property. Anyway, the code that did this checks was very difficult to read, so let's take the opportunity to refactor it, with no functional changes. https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1728 --- src/libnmc-setting/nm-meta-setting-desc.c | 131 +++++++++------------- src/libnmc-setting/nm-meta-setting-desc.h | 2 +- 2 files changed, 51 insertions(+), 82 deletions(-) diff --git a/src/libnmc-setting/nm-meta-setting-desc.c b/src/libnmc-setting/nm-meta-setting-desc.c index 2b2209d9f9..4a6626d039 100644 --- a/src/libnmc-setting/nm-meta-setting-desc.c +++ b/src/libnmc-setting/nm-meta-setting-desc.c @@ -1071,21 +1071,19 @@ _get_fcn_gobject_secret_flags(ARGS_GET_FCN) static gconstpointer _get_fcn_gobject_enum(ARGS_GET_FCN) { - GType gtype = 0; - nm_auto_unref_gtypeclass GTypeClass *gtype_class = NULL; - nm_auto_unref_gtypeclass GTypeClass *gtype_prop_class = NULL; - const NMUtilsEnumValueInfo *value_infos = NULL; - gboolean has_gtype = FALSE; - nm_auto_unset_gvalue GValue gval = G_VALUE_INIT; - gint64 v; - gboolean format_numeric = FALSE; - gboolean format_numeric_hex = FALSE; - gboolean format_numeric_hex_unknown = FALSE; - gboolean format_text = FALSE; - gboolean format_text_l10n = FALSE; - gs_free char *s = NULL; - char s_numeric[64]; - GParamSpec *pspec; + GType gtype = 0; + const NMUtilsEnumValueInfo *value_infos = NULL; + gboolean has_gtype = FALSE; + nm_auto_unset_gvalue GValue gval = G_VALUE_INIT; + gint64 v; + gboolean format_numeric = FALSE; + gboolean format_numeric_hex = FALSE; + gboolean format_numeric_hex_unknown = FALSE; + gboolean format_text = FALSE; + gboolean format_text_l10n = FALSE; + gs_free char *s = NULL; + char s_numeric[64]; + GParamSpec *pspec; RETURN_UNSUPPORTED_GET_TYPE(); @@ -1141,50 +1139,32 @@ _get_fcn_gobject_enum(ARGS_GET_FCN) pspec = g_object_class_find_property(G_OBJECT_GET_CLASS(setting), property_info->property_name); g_return_val_if_fail(pspec, NULL); + if (has_gtype) { + /* if the property is already enum, don't set get_gtype: it's redundant and error prone */ + g_return_val_if_fail(NM_IN_SET(pspec->value_type, G_TYPE_INT, G_TYPE_UINT), FALSE); + } else { + gtype = pspec->value_type; + } + + g_return_val_if_fail(G_TYPE_IS_ENUM(gtype) || G_TYPE_IS_FLAGS(gtype), NULL); + g_value_init(&gval, pspec->value_type); g_object_get_property(G_OBJECT(setting), property_info->property_name, &gval); NM_SET_OUT(out_is_default, g_param_value_defaults(pspec, &gval)); - if (pspec->value_type == G_TYPE_INT - || (G_TYPE_IS_CLASSED(pspec->value_type) - && G_IS_ENUM_CLASS( - (gtype_prop_class ?: (gtype_prop_class = g_type_class_ref(pspec->value_type)))))) { - if (pspec->value_type == G_TYPE_INT) { - if (!has_gtype) - g_return_val_if_reached(NULL); - v = g_value_get_int(&gval); - } else - v = g_value_get_enum(&gval); - } else if (pspec->value_type == G_TYPE_UINT - || (G_TYPE_IS_CLASSED(pspec->value_type) - && G_IS_FLAGS_CLASS( - (gtype_prop_class - ?: (gtype_prop_class = g_type_class_ref(pspec->value_type)))))) { - if (pspec->value_type == G_TYPE_UINT) { - if (!has_gtype) - g_return_val_if_reached(NULL); - v = g_value_get_uint(&gval); - } else - v = g_value_get_flags(&gval); - } else + if (pspec->value_type == G_TYPE_INT) + v = g_value_get_int(&gval); + else if (pspec->value_type == G_TYPE_UINT) + v = g_value_get_uint(&gval); + else if (G_TYPE_IS_ENUM(pspec->value_type)) + v = g_value_get_enum(&gval); + else if (G_TYPE_IS_FLAGS(pspec->value_type)) + v = g_value_get_flags(&gval); + else g_return_val_if_reached(NULL); - if (!has_gtype) { - gtype = pspec->value_type; - gtype_class = g_steal_pointer(>ype_prop_class); - } - - nm_assert(({ - nm_auto_unref_gtypeclass GTypeClass *t = NULL; - - (G_TYPE_IS_CLASSED(gtype) && (t = g_type_class_ref(gtype)) - && (G_IS_ENUM_CLASS(t) || G_IS_FLAGS_CLASS(t))); - })); - if (format_numeric && !format_text) { - s = format_numeric_hex - || (format_numeric_hex_unknown - && !G_IS_ENUM_CLASS(gtype_class ?: (gtype_class = g_type_class_ref(gtype)))) + s = format_numeric_hex || (format_numeric_hex_unknown && !G_TYPE_IS_ENUM(gtype)) ? g_strdup_printf("0x%" G_GINT64_MODIFIER "x", v) : g_strdup_printf("%" G_GINT64_FORMAT, v); RETURN_STR_TO_FREE(g_steal_pointer(&s)); @@ -1201,9 +1181,7 @@ _get_fcn_gobject_enum(ARGS_GET_FCN) if (!format_numeric) RETURN_STR_TO_FREE(g_steal_pointer(&s)); - if (format_numeric_hex - || (format_numeric_hex_unknown - && !G_IS_ENUM_CLASS(gtype_class ?: (gtype_class = g_type_class_ref(gtype))))) + if (format_numeric_hex || (format_numeric_hex_unknown && !G_TYPE_IS_ENUM(gtype))) nm_sprintf_buf(s_numeric, "0x%" G_GINT64_MODIFIER "x", v); else nm_sprintf_buf(s_numeric, "%" G_GINT64_FORMAT, v); @@ -1597,14 +1575,12 @@ _set_fcn_gobject_mac(ARGS_SET_FCN) static gboolean _set_fcn_gobject_enum(ARGS_SET_FCN) { - GType gtype = 0; - GType gtype_prop; - gboolean has_gtype = FALSE; - nm_auto_unset_gvalue GValue gval = G_VALUE_INIT; - nm_auto_unref_gtypeclass GTypeClass *gtype_prop_class = NULL; - nm_auto_unref_gtypeclass GTypeClass *gtype_class = NULL; - gboolean is_flags; - int v; + GType gtype = 0; + GType gtype_prop; + gboolean has_gtype = FALSE; + nm_auto_unset_gvalue GValue gval = G_VALUE_INIT; + gboolean is_flags; + int v; if (_SET_FCN_DO_RESET_DEFAULT_WITH_SUPPORTS_REMOVE(property_info, modifier, value)) return _gobject_property_reset_default(setting, property_info->property_name); @@ -1618,17 +1594,15 @@ _set_fcn_gobject_enum(ARGS_SET_FCN) gtype_prop = _gobject_property_get_gtype(G_OBJECT(setting), property_info->property_name); - if (has_gtype && NM_IN_SET(gtype_prop, G_TYPE_INT, G_TYPE_UINT) && G_TYPE_IS_CLASSED(gtype) - && (gtype_prop_class = g_type_class_ref(gtype)) - && ((is_flags = G_IS_FLAGS_CLASS(gtype_prop_class)) || G_IS_ENUM_CLASS(gtype_prop_class))) { - /* valid */ - } else if (!has_gtype && G_TYPE_IS_CLASSED(gtype_prop) - && (gtype_prop_class = g_type_class_ref(gtype_prop)) - && ((is_flags = G_IS_FLAGS_CLASS(gtype_prop_class)) - || G_IS_ENUM_CLASS(gtype_prop_class))) { + if (has_gtype) { + /* if the property is already enum, don't set get_gtype: it's redundant and error prone */ + g_return_val_if_fail(NM_IN_SET(gtype_prop, G_TYPE_INT, G_TYPE_UINT), FALSE); + } else { gtype = gtype_prop; - } else - g_return_val_if_reached(FALSE); + } + + g_return_val_if_fail(G_TYPE_IS_FLAGS(gtype) || G_TYPE_IS_ENUM(gtype), FALSE); + is_flags = G_TYPE_IS_FLAGS(gtype); if (!_nm_utils_enum_from_str_full( gtype, @@ -1649,9 +1623,7 @@ _set_fcn_gobject_enum(ARGS_SET_FCN) v); } - gtype_class = g_type_class_ref(gtype); - - if (G_IS_FLAGS_CLASS(gtype_class) && !_SET_FCN_DO_SET_ALL(modifier, value)) { + if (is_flags && !_SET_FCN_DO_SET_ALL(modifier, value)) { nm_auto_unset_gvalue GValue int_value = {}; guint v_flag; @@ -1670,13 +1642,10 @@ _set_fcn_gobject_enum(ARGS_SET_FCN) g_value_set_int(&gval, v); else if (gtype_prop == G_TYPE_UINT) g_value_set_uint(&gval, v); - else if (is_flags) { - nm_assert(G_IS_FLAGS_CLASS(gtype_prop_class)); + else if (is_flags) g_value_set_flags(&gval, v); - } else { - nm_assert(G_IS_ENUM_CLASS(gtype_prop_class)); + else g_value_set_enum(&gval, v); - } if (!nm_g_object_set_property(G_OBJECT(setting), property_info->property_name, &gval, NULL)) goto fail; diff --git a/src/libnmc-setting/nm-meta-setting-desc.h b/src/libnmc-setting/nm-meta-setting-desc.h index d27c64fe5e..94a689457c 100644 --- a/src/libnmc-setting/nm-meta-setting-desc.h +++ b/src/libnmc-setting/nm-meta-setting-desc.h @@ -277,7 +277,7 @@ typedef struct { struct _NMMetaPropertyTypData { union { struct { - GType (*get_gtype)(void); + GType (*get_gtype)(void); /* note: only allowed for int/uint properties */ int min; int max; const struct _NMUtilsEnumValueInfo *value_infos_get; /* nicks for get function */