[Libosinfo] [libosinfo PATCH 1/7] entity: Add methods to deal with GFlags

[Daniel P. Berrangé]

On Mon, Dec 03, 2018 at 10:11:46AM +0100, Fabiano Fidêncio wrote:
> Add osinfo_entity_get_param_value_flags() and
> osinfo_entity_set_param_flags() in order to deal with flags as such
> OsinfoInstallScriptMethods.
> 
> Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
> ---
>  osinfo/libosinfo.syms  |  3 +++
>  osinfo/osinfo_entity.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  osinfo/osinfo_entity.h |  5 +++++
>  3 files changed, 49 insertions(+)
> 
> diff --git a/osinfo/libosinfo.syms b/osinfo/libosinfo.syms
> index 5396c72..365914c 100644
> --- a/osinfo/libosinfo.syms
> +++ b/osinfo/libosinfo.syms
> @@ -531,6 +531,9 @@ LIBOSINFO_0.2.13 {
>  
>  LIBOSINFO_1.3.0 {
>      global:
> +	osinfo_entity_get_param_value_flags;
> +	osinfo_entity_set_param_flags;
> +
>  	osinfo_error_quark;
>  
>  	osinfo_os_add_maximum_resources;
> diff --git a/osinfo/osinfo_entity.c b/osinfo/osinfo_entity.c
> index 20b9115..563432a 100644
> --- a/osinfo/osinfo_entity.c
> +++ b/osinfo/osinfo_entity.c
> @@ -221,6 +221,22 @@ void osinfo_entity_set_param_enum(OsinfoEntity *entity, const gchar *key, gint v
>      osinfo_entity_set_param(entity, key, enum_value->value_nick);
>  }
>  
> +void osinfo_entity_set_param_flags(OsinfoEntity *entity, const gchar *key, guint value, GType flags_type)
> +{
> +    GFlagsClass *flags_class;
> +    guint i;
> +
> +    g_return_if_fail(G_TYPE_IS_FLAGS(flags_type));
> +
> +    flags_class = g_type_class_ref(flags_type);
> +    for (i = 0; i < flags_class->n_values; i++) {
> +        if ((flags_class->values[i].value & value) != 0) {
> +            osinfo_entity_set_param(entity, key, flags_class->values[i].value_nick);
> +            break;
> +        }

GFlag is a type for using an enum as a bitmask. As such the
value can have multiple bits set. This code however breaks
as soon as a single matching flag bit is found, and ignores
all following bits.

This doesn't feel right to me.

Why do we need to use GFlag if we're not going to honour
more than one bit being set ? Doesn't that just become
an enum at that point

> +    }
> +}
> +
>  /**
>   * osinfo_entity_add_param:
>   * @entity: an #OsinfoEntity containing the parameters
> @@ -401,6 +417,31 @@ gint osinfo_entity_get_param_value_enum(OsinfoEntity *entity,
>      g_return_val_if_reached(default_value);
>  }
>  
> +guint osinfo_entity_get_param_value_flags(OsinfoEntity *entity,
> +                                          const char *key,
> +                                          GType flags_type,
> +                                          guint default_value)
> +{
> +    const gchar *nick;
> +    GFlagsClass *flags_class;
> +    GFlagsValue *flags_value;
> +
> +    g_return_val_if_fail(G_TYPE_IS_FLAGS(flags_type), default_value);
> +
> +    nick = osinfo_entity_get_param_value(entity, key);
> +    if (nick == NULL)
> +        return default_value;
> +
> +    flags_class = g_type_class_ref(flags_type);
> +    flags_value = g_flags_get_value_by_nick(flags_class, nick);
> +    g_type_class_unref(flags_class);
> +
> +    if (flags_value != NULL)
> +        return flags_value->value;
> +
> +    g_return_val_if_reached(default_value);
> +}
> +
>  /**
>   * osinfo_entity_get_param_value_list:
>   * @entity: an #OsinfoEntity containing the parameters
> diff --git a/osinfo/osinfo_entity.h b/osinfo/osinfo_entity.h
> index d41237b..f4ed465 100644
> --- a/osinfo/osinfo_entity.h
> +++ b/osinfo/osinfo_entity.h
> @@ -79,6 +79,10 @@ gint osinfo_entity_get_param_value_enum(OsinfoEntity *entity,
>                                          const char *key,
>                                          GType enum_type,
>                                          gint default_value);
> +guint osinfo_entity_get_param_value_flags(OsinfoEntity *entity,
> +                                           const char *key,
> +                                           GType flags_type,
> +                                           guint default_value);
>  gint64 osinfo_entity_get_param_value_int64(OsinfoEntity *entity, const gchar *key);
>  gint64 osinfo_entity_get_param_value_int64_with_default(OsinfoEntity *entity,
>                                                          const gchar *key,
> @@ -88,6 +92,7 @@ void osinfo_entity_set_param(OsinfoEntity *entity, const gchar *key, const gchar
>  void osinfo_entity_set_param_boolean(OsinfoEntity *entity, const gchar *key, gboolean value);
>  void osinfo_entity_set_param_int64(OsinfoEntity *entity, const gchar *key, gint64 value);
>  void osinfo_entity_set_param_enum(OsinfoEntity *entity, const gchar *key, gint value, GType enum_type);
> +void osinfo_entity_set_param_flags(OsinfoEntity *entity, const gchar *key, guint value, GType flags_type);
>  void osinfo_entity_add_param(OsinfoEntity *entity, const gchar *key, const gchar *value);
>  void osinfo_entity_clear_param(OsinfoEntity *entity, const gchar *key);

I'm not entirely convinced 

>  
> -- 
> 2.19.1
> 
> _______________________________________________
> Libosinfo mailing list
> Libosinfo at redhat.com
> https://www.redhat.com/mailman/listinfo/libosinfo

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|