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@xxxxxxxxxx> 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
Attachment:
pgpVajWIR3yDI.pgp
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
