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

[Daniel P. Berrangé]

On Fri, Dec 14, 2018 at 11:24:08AM +0100, Fabiano Fidêncio wrote:
> [snip]
> 
> > >
> > > +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.
> 
> Hmm. True. I'll fix this if we decide to take this path ...
> >
> > 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
> 
> I agree. There's one thing thought that, IIRC, made me take this path.
> I wasn't willing to add a new enum for the possible injection methods
> and I cannot use the current OsinfoInstallScriptInjectionMethod enum
> as it's treated as a GFlags. Here's the generated code for it:
> 
>  66 GType
>  67 osinfo_install_script_injection_method_get_type (void)
>  68 {
>  69   static volatile gsize g_define_type_id__volatile = 0;
>  70
>  71   if (g_once_init_enter (&g_define_type_id__volatile))
>  72     {
>  73       static const GFlagsValue values[] = {
>  74         { OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_CDROM,
> "OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_CDROM", "cdrom" },
>  75         { OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_DISK,
> "OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_DISK", "disk" },
>  76         { OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_FLOPPY,
> "OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_FLOPPY", "floppy" },
>  77         { OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_INITRD,
> "OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_INITRD", "initrd" },
>  78         { OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_WEB,
> "OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_WEB", "web" },
>  79         { 0, NULL, NULL }
>  80       };
>  81       GType g_define_type_id =
>  82         g_flags_register_static (g_intern_static_string
> ("OsinfoInstallScriptInjectionMethod"), values);
>  83       g_once_init_leave (&g_define_type_id__volatile, g_define_type_id);
>  84     }
>  85
>  86   return g_define_type_id__volatile;
>  87 }
> 
> So, here we basically have two ways of doing this:
> - Using the pre-existent GFlags we have (and then fixing this code);

Use GFlags, but either somehow handle multiple values, or just
return an error if multiple values are set.

> - Creating a new enum (that won't be a flag) and be happy with it.
> 
> What's your preference?
> [snip]

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 :|