On Tue, Dec 11, 2012 at 04:11:53PM +0200, Zeeshan Ali (Khattak) wrote:
> On Tue, Dec 11, 2012 at 11:52 AM, Christophe Fergeau
> <cfergeau@xxxxxxxxxx> 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@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 ;)
>
> 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.
Yep, I can add a comment. Not sure where to mention that in the
documentation as IDs don't appear anywhere in install script parameters.
To be honest, this API would fit better in OsinfoInstallScriptParamsList.
Christophe
Attachment:
pgpbxVIp8ZOKL.pgp
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
