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 :|
_______________________________________________
Libosinfo mailing list
Libosinfo@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libosinfo
