[virt-tools-list] [libosinfo 07/11] OsinfoInstallScript: Use an OsinfoInstallConfigParamList

[Zeeshan Ali (Khattak)]

On Tue, Dec 11, 2012 at 11:52 AM, Christophe Fergeau
<cfergeau at redhat.com> wrote:
> On Tue, Dec 11, 2012 at 12:20:09AM +0200, Zeeshan Ali (Khattak) wrote:
>> On Mon, Dec 10, 2012 at 6:46 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
>> >  gboolean osinfo_install_script_has_config_param_name(const OsinfoInstallScript *script, const gchar *name)
>> >  {
>> > -    GList *l;
>> > -
>> > -    for (l = script->priv->config_param_list; l != NULL; l = l->next) {
>> > -        OsinfoInstallConfigParam *tmp = l->data;
>> > -
>> > -        if (g_strcmp0(osinfo_install_config_param_get_name(tmp), name) == 0)
>> > -            return TRUE;
>> > -    }
>> > -    return FALSE;
>> > +    OsinfoList *l = OSINFO_LIST(script->priv->config_params);
>> > +    return (osinfo_list_find_by_id(l, name) != NULL);
>>
>> Should this code be assuming that name and ID are the same thing for
>> ConfigParam?
>
> This is discussed a bit in 'Set id in osinfo_install_config_param_new'
> commit log. OsinfoInstallConfigParam should have had an id from the start
> as it's an entity, but it currently does not. The patch mentioned above
> uses the name as id.
> I'm fine with changing the code from these various functions back to list
> iterations so that we don't rely on 'id' and 'name' being the same, but I
> hate code doing lookup in lists ;)

You are not alone. :) I was only thinking aloud. Do as you think is
best but if you go with this approach, perhaps adding this fact to
OsinfoInstallConfigParam documentation might be a good idea (if you
haven't done that already) and a comment in here as well.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124