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

[Christophe Fergeau]

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 ;)

> > +/**
> > + * osinfo_install_script_get_config_paramlist:
> > + *
> > + * Get the list of valid config parameters for @script.
> > + *
> > + * Returns: (transfer none): the
> > + * list of valid #OsinfoInstallConfigParam parameters. Free with
> > + * g_list_free() when done. The elements are owned by libosinfo.
> 
> You want to update the doc about free function above.

Thanks, changed.

> 
> > + */
> > +OsinfoInstallConfigParamList *osinfo_install_script_get_config_paramlist(const OsinfoInstallScript *script)
> 
> This name seems a bit inconsistent with names for existing similar
> functions. Since _get_config_param_list() is already taken, I suggest
> _get_config_params.

And changed too.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20121211/ca467abf/attachment.sig>